From 87b84011b6fd1690c3ca2cf08a7b4b38a0cfcea5 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Sun, 9 Feb 2025 15:49:25 +0100 Subject: [PATCH 1/3] fix(capabilities): make a deep comparison - need to check if there were actual changes that require a reload Signed-off-by: Maksim Sukharev --- src/services/CapabilitiesManager.ts | 59 ++++++++++++++++--- .../__tests__/CapabilitiesManager.spec.js | 22 ++++++- src/test-setup.js | 2 + 3 files changed, 73 insertions(+), 10 deletions(-) diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index 015ab4260ec..ebd952e0b94 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -14,7 +14,7 @@ import type { Capabilities, Conversation, JoinRoomFullResponse } from '../types/ import { messagePleaseReload } from '../utils/talkDesktopUtils.ts' type Config = Capabilities['spreed']['config'] -type RemoteCapability = Capabilities & Partial<{ hash: string }> +type RemoteCapability = Capabilities & { hash?: string } type RemoteCapabilities = Record type TokenMap = Record @@ -114,7 +114,7 @@ function getRemoteCapability(token: string): RemoteCapability | null { */ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullResponse): Promise { const token = joinRoomResponse.data.ocs.data.token - const remoteServer = joinRoomResponse.data.ocs.data.remoteServer as string + const remoteServer = joinRoomResponse.data.ocs.data.remoteServer! // Check if remote capabilities have not changed since last check if (joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] === remoteCapabilities[remoteServer]?.hash) { @@ -126,20 +126,61 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon talkHashStore.setTalkProxyHashDirty(token) const response = await getRemoteCapabilities(token) - if (!Object.keys(response.data.ocs.data).length) { + const newRemoteCapabilities = response.data.ocs.data as Capabilities['spreed'] + if (!Object.keys(newRemoteCapabilities).length) { // data: {} received from server, nothing to update with return } - remoteCapabilities[remoteServer] = { spreed: (response.data.ocs.data as Capabilities['spreed']) } - remoteCapabilities[remoteServer].hash = joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] + const shouldShowWarning = checkRemoteCapabilitiesHasChanged(newRemoteCapabilities, remoteCapabilities[remoteServer]?.spreed) + remoteCapabilities[remoteServer] = { + spreed: newRemoteCapabilities, + hash: joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'], + } BrowserStorage.setItem('remoteCapabilities', JSON.stringify(remoteCapabilities)) patchTokenMap(joinRoomResponse.data.ocs.data) - // As normal capabilities update, requires a reload to take effect - showError(t('spreed', 'Nextcloud Talk Federation was updated.') + '\n' + messagePleaseReload, { - timeout: TOAST_PERMANENT_TIMEOUT, - }) + if (shouldShowWarning) { + // As normal capabilities update, requires a reload to take effect + showError(t('spreed', 'Nextcloud Talk Federation was updated.') + '\n' + messagePleaseReload, { + timeout: TOAST_PERMANENT_TIMEOUT, + }) + } +} + +/** + * Deep comparison of remote capabilities, whether there are actual changes that require reload + * @param newObject new remote capabilities + * @param oldObject old remote capabilities + */ +function checkRemoteCapabilitiesHasChanged(newObject: Capabilities['spreed'], oldObject: Capabilities['spreed']): boolean { + if (!newObject || !oldObject) { + return true + } + + /** + * Returns remote config without local-only properties + * @param object remote capabilities object + */ + function getStrippedCapabilities(object: Capabilities['spreed']): { config: Partial, features: string[] } { + const config = structuredClone(object.config) + + for (const key1 of Object.keys(object['config-local']) as Array) { + const keys2 = object['config-local'][key1] + for (const key2 of keys2 as Array) { + delete config[key1][key2] + } + if (!Object.keys(config[key1]).length) { + delete config[key1] + } + } + + const features = object.features.filter(feature => !object['features-local'].includes(feature)).sort() + + return { config, features } + } + + return JSON.stringify(getStrippedCapabilities(newObject)) !== JSON.stringify(getStrippedCapabilities(oldObject)) } /** diff --git a/src/services/__tests__/CapabilitiesManager.spec.js b/src/services/__tests__/CapabilitiesManager.spec.js index 6533bf43cf5..45b70326270 100644 --- a/src/services/__tests__/CapabilitiesManager.spec.js +++ b/src/services/__tests__/CapabilitiesManager.spec.js @@ -146,12 +146,32 @@ describe('CapabilitiesManager', () => { expect(BrowserStorage.setItem).toHaveBeenCalledTimes(0) }) + it('should set capabilities for new server and mark talk proxy hash as dirty', async () => { + const token = 'TOKEN7FED3' + const remoteServer = 'https://nextcloud3.local' + const remoteHash = 'abc123' + const joinRoomResponseMock = generateOCSResponse({ + headers: { 'x-nextcloud-talk-proxy-hash': remoteHash }, + payload: { token, remoteServer } + }) + const responseMock = generateOCSResponse({ payload: mockedCapabilities.spreed }) + getRemoteCapabilities.mockReturnValue(responseMock) + await setRemoteCapabilities(joinRoomResponseMock) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(1) + }) + it('should update capabilities from server response and mark talk proxy hash as dirty', async () => { const joinRoomResponseMock = generateOCSResponse({ headers: { 'x-nextcloud-talk-proxy-hash': `${remoteCapabilities.hash}002` }, payload: { token, remoteServer } }) - const responseMock = generateOCSResponse({ payload: mockedCapabilities.spreed }) + const responseMock = generateOCSResponse({ + payload: { + ...mockedCapabilities.spreed, + features: [...mockedCapabilities.spreed.features, 'new-feature'], + } + }) getRemoteCapabilities.mockReturnValue(responseMock) await setRemoteCapabilities(joinRoomResponseMock) expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() diff --git a/src/test-setup.js b/src/test-setup.js index ccc6136592a..f8a4349a934 100644 --- a/src/test-setup.js +++ b/src/test-setup.js @@ -115,6 +115,8 @@ global.ResizeObserver = jest.fn(() => ({ disconnect: jest.fn(), })) +global.structuredClone = jest.fn((val) => JSON.parse(JSON.stringify(val))) + // Work around missing "URL.createObjectURL" (which is used in the code but not // relevant for the tests) in jsdom: https://github.com/jsdom/jsdom/issues/1721 window.URL.createObjectURL = jest.fn() From 5881c0b67dfb7b2213da25c2a823961bcb356971 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Sun, 9 Feb 2025 16:19:54 +0100 Subject: [PATCH 2/3] fix(capabilities): clear dirty hash on rejoin Signed-off-by: Maksim Sukharev --- src/components/TopBar/CallButton.vue | 3 +- src/services/CapabilitiesManager.ts | 13 ++++---- .../__tests__/CapabilitiesManager.spec.js | 31 +++++++++++++++++++ src/stores/talkHash.js | 24 ++++++++++++++ 4 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/components/TopBar/CallButton.vue b/src/components/TopBar/CallButton.vue index 00aa567bacf..0c342317bc8 100644 --- a/src/components/TopBar/CallButton.vue +++ b/src/components/TopBar/CallButton.vue @@ -353,8 +353,9 @@ export default { }, watch: { - token() { + token(newValue, oldValue) { this.callViewStore.resetCallHasJustEnded() + this.talkHashStore.resetTalkProxyHashDirty(oldValue) } }, diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index ebd952e0b94..2f686c2bb13 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -4,14 +4,11 @@ */ import { getCapabilities as _getCapabilities } from '@nextcloud/capabilities' -import { showError, TOAST_PERMANENT_TIMEOUT } from '@nextcloud/dialogs' -import { t } from '@nextcloud/l10n' import { getRemoteCapabilities } from './federationService.ts' import BrowserStorage from '../services/BrowserStorage.js' import { useTalkHashStore } from '../stores/talkHash.js' import type { Capabilities, Conversation, JoinRoomFullResponse } from '../types/index.ts' -import { messagePleaseReload } from '../utils/talkDesktopUtils.ts' type Config = Capabilities['spreed']['config'] type RemoteCapability = Capabilities & { hash?: string } @@ -113,16 +110,18 @@ function getRemoteCapability(token: string): RemoteCapability | null { * @param joinRoomResponse server response */ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullResponse): Promise { + const talkHashStore = useTalkHashStore() + const token = joinRoomResponse.data.ocs.data.token const remoteServer = joinRoomResponse.data.ocs.data.remoteServer! // Check if remote capabilities have not changed since last check if (joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] === remoteCapabilities[remoteServer]?.hash) { + talkHashStore.resetTalkProxyHashDirty(token) return } // Mark the hash as dirty to prevent any activity in the conversation - const talkHashStore = useTalkHashStore() talkHashStore.setTalkProxyHashDirty(token) const response = await getRemoteCapabilities(token) @@ -142,9 +141,9 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon if (shouldShowWarning) { // As normal capabilities update, requires a reload to take effect - showError(t('spreed', 'Nextcloud Talk Federation was updated.') + '\n' + messagePleaseReload, { - timeout: TOAST_PERMANENT_TIMEOUT, - }) + talkHashStore.showTalkProxyHashDirtyToast() + } else { + talkHashStore.resetTalkProxyHashDirty(token) } } diff --git a/src/services/__tests__/CapabilitiesManager.spec.js b/src/services/__tests__/CapabilitiesManager.spec.js index 45b70326270..7cc136fce09 100644 --- a/src/services/__tests__/CapabilitiesManager.spec.js +++ b/src/services/__tests__/CapabilitiesManager.spec.js @@ -177,5 +177,36 @@ describe('CapabilitiesManager', () => { expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() expect(BrowserStorage.setItem).toHaveBeenCalledTimes(1) }) + + it('should reset dirty proxy hash after second fetch and negative check for changes', async () => { + const joinRoomResponseMock = generateOCSResponse({ + headers: { 'x-nextcloud-talk-proxy-hash': `${remoteCapabilities.hash}003` }, + payload: { token, remoteServer } + }) + const joinRoomResponseMock2 = generateOCSResponse({ + headers: { 'x-nextcloud-talk-proxy-hash': `${remoteCapabilities.hash}004` }, + payload: { token, remoteServer } + }) + const responseMock = generateOCSResponse({ + payload: { + ...mockedCapabilities.spreed, + features: [...mockedCapabilities.spreed.features, 'new-feature', 'new-feature-2'], + } + }) + const responseMock2 = generateOCSResponse({ + payload: { + ...mockedCapabilities.spreed, + features: [...mockedCapabilities.spreed.features, 'new-feature', 'new-feature-2'], + } + }) + getRemoteCapabilities.mockReturnValueOnce(responseMock).mockReturnValueOnce(responseMock2) + await setRemoteCapabilities(joinRoomResponseMock) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(1) + + await setRemoteCapabilities(joinRoomResponseMock2) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).not.toBeDefined() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(2) + }) }) }) diff --git a/src/stores/talkHash.js b/src/stores/talkHash.js index 3d9396daedc..e3bf9313dc1 100644 --- a/src/stores/talkHash.js +++ b/src/stores/talkHash.js @@ -32,6 +32,7 @@ export const useTalkHashStore = defineStore('talkHash', { isNextcloudTalkHashDirty: false, isNextcloudTalkProxyHashDirty: {}, maintenanceWarningToast: null, + proxyHashDirtyToast: null, }), actions: { @@ -60,6 +61,20 @@ export const useTalkHashStore = defineStore('talkHash', { Vue.set(this.isNextcloudTalkProxyHashDirty, token, true) }, + /** + * Clear the current Talk Federation dirty hash marker (as it is irrelevant after reload or leaving) + * + * @param {string} token federated conversation token + */ + resetTalkProxyHashDirty(token) { + Vue.delete(this.isNextcloudTalkProxyHashDirty, token) + + if (this.proxyHashDirtyToast) { + this.proxyHashDirtyToast.hideToast() + this.proxyHashDirtyToast = null + } + }, + /** * Updates a Talk hash from a response * @@ -104,5 +119,14 @@ export const useTalkHashStore = defineStore('talkHash', { this.maintenanceWarningToast = null } }, + + /** + * Show a toast message when Talk Federation requires rejoining + */ + showTalkProxyHashDirtyToast() { + this.proxyHashDirtyToast = showError(t('spreed', 'Nextcloud Talk Federation was updated.') + '\n' + messagePleaseReload, { + timeout: TOAST_PERMANENT_TIMEOUT, + }) + }, } }) From 691a9aa1348057cea6d2021a980d911a1de903f1 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Sun, 9 Feb 2025 17:14:55 +0100 Subject: [PATCH 3/3] fix(capabilities): fetch remote capabilities for unknown server Signed-off-by: Maksim Sukharev --- src/services/CapabilitiesManager.ts | 27 +++++++++++++- .../__tests__/CapabilitiesManager.spec.js | 36 +++++++++++++++++++ src/stores/__tests__/federation.spec.js | 7 +++- src/stores/federation.ts | 2 ++ 4 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index 2f686c2bb13..70f512f8aff 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -8,7 +8,7 @@ import { getCapabilities as _getCapabilities } from '@nextcloud/capabilities' import { getRemoteCapabilities } from './federationService.ts' import BrowserStorage from '../services/BrowserStorage.js' import { useTalkHashStore } from '../stores/talkHash.js' -import type { Capabilities, Conversation, JoinRoomFullResponse } from '../types/index.ts' +import type { acceptShareResponse, Capabilities, Conversation, JoinRoomFullResponse } from '../types/index.ts' type Config = Capabilities['spreed']['config'] type RemoteCapability = Capabilities & { hash?: string } @@ -147,6 +147,31 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon } } +/** + * Fetch new capabilities if remote server is not yet known + * @param acceptShareResponse server response + */ +export async function setRemoteCapabilitiesIfEmpty(acceptShareResponse: Awaited): Promise { + const token = acceptShareResponse.data.ocs.data.token + const remoteServer = acceptShareResponse.data.ocs.data.remoteServer! + + // Check if remote capabilities already exists + if (remoteCapabilities[remoteServer]) { + return + } + + const response = await getRemoteCapabilities(token) + const newRemoteCapabilities = response.data.ocs.data as Capabilities['spreed'] + if (!Object.keys(newRemoteCapabilities).length) { + // data: {} received from server, nothing to update with + return + } + + remoteCapabilities[remoteServer] = { spreed: newRemoteCapabilities } + BrowserStorage.setItem('remoteCapabilities', JSON.stringify(remoteCapabilities)) + patchTokenMap(acceptShareResponse.data.ocs.data) +} + /** * Deep comparison of remote capabilities, whether there are actual changes that require reload * @param newObject new remote capabilities diff --git a/src/services/__tests__/CapabilitiesManager.spec.js b/src/services/__tests__/CapabilitiesManager.spec.js index 7cc136fce09..8375f9e8db0 100644 --- a/src/services/__tests__/CapabilitiesManager.spec.js +++ b/src/services/__tests__/CapabilitiesManager.spec.js @@ -12,6 +12,7 @@ import { hasTalkFeature, getTalkConfig, setRemoteCapabilities, + setRemoteCapabilitiesIfEmpty, } from '../CapabilitiesManager.ts' import { getRemoteCapabilities } from '../federationService.ts' @@ -209,4 +210,39 @@ describe('CapabilitiesManager', () => { expect(BrowserStorage.setItem).toHaveBeenCalledTimes(2) }) }) + + describe('setRemoteCapabilitiesIfEmpty', () => { + const token = 'TOKEN8FED4' + const remoteServer = 'https://nextcloud4.local' + + it('should early return if already has capabilities', async () => { + const [remoteServer, remoteCapabilities] = Object.entries(mockedRemotes)[0] + const token = remoteCapabilities.tokens[0] + const acceptShareResponseMock = generateOCSResponse({ + payload: { token, remoteServer }, + }) + await setRemoteCapabilitiesIfEmpty(acceptShareResponseMock) + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(0) + }) + + it('should early return if no capabilities received from server', async () => { + const acceptShareResponseMock = generateOCSResponse({ + payload: { token, remoteServer }, + }) + const responseMock = generateOCSResponse({ payload: {} }) + getRemoteCapabilities.mockReturnValue(responseMock) + await setRemoteCapabilitiesIfEmpty(acceptShareResponseMock) + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(0) + }) + + it('should set capabilities for new server', async () => { + const acceptShareResponseMock = generateOCSResponse({ + payload: { token, remoteServer }, + }) + const responseMock = generateOCSResponse({ payload: mockedCapabilities.spreed }) + getRemoteCapabilities.mockReturnValue(responseMock) + await setRemoteCapabilitiesIfEmpty(acceptShareResponseMock) + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(1) + }) + }) }) diff --git a/src/stores/__tests__/federation.spec.js b/src/stores/__tests__/federation.spec.js index 3aae72463e6..22ebf6cf5cd 100644 --- a/src/stores/__tests__/federation.spec.js +++ b/src/stores/__tests__/federation.spec.js @@ -4,7 +4,8 @@ */ import { setActivePinia, createPinia } from 'pinia' -import { getShares, acceptShare, rejectShare } from '../../services/federationService.ts' +import { mockedCapabilities } from '../../__mocks__/capabilities.ts' +import { getShares, acceptShare, rejectShare, getRemoteCapabilities } from '../../services/federationService.ts' import { generateOCSErrorResponse, generateOCSResponse } from '../../test-helpers.js' import { useFederationStore } from '../federation.ts' @@ -12,6 +13,7 @@ jest.mock('../../services/federationService', () => ({ getShares: jest.fn(), acceptShare: jest.fn(), rejectShare: jest.fn(), + getRemoteCapabilities: jest.fn(), })) describe('federationStore', () => { @@ -187,6 +189,9 @@ describe('federationStore', () => { const acceptResponse = generateOCSResponse({ payload: room }) acceptShare.mockResolvedValueOnce(acceptResponse) + const responseMock = generateOCSResponse({ payload: mockedCapabilities.spreed }) + getRemoteCapabilities.mockReturnValue(responseMock) + // Act: accept invite const conversation = await federationStore.acceptShare(invites[0].id) diff --git a/src/stores/federation.ts b/src/stores/federation.ts index 9baac76ef67..393db211cf0 100644 --- a/src/stores/federation.ts +++ b/src/stores/federation.ts @@ -11,6 +11,7 @@ import { t } from '@nextcloud/l10n' import { getBaseUrl } from '@nextcloud/router' import { FEDERATION } from '../constants.ts' +import { setRemoteCapabilitiesIfEmpty } from '../services/CapabilitiesManager.ts' import { getShares, acceptShare, rejectShare } from '../services/federationService.ts' import type { Conversation, FederationInvite, NotificationInvite } from '../types/index.ts' @@ -109,6 +110,7 @@ export const useFederationStore = defineStore('federation', { try { Vue.set(this.pendingShares[id], 'loading', 'accept') const response = await acceptShare(id) + await setRemoteCapabilitiesIfEmpty(response) this.markInvitationAccepted(id, response.data.ocs.data) this.updatePendingSharesCount(Object.keys(this.pendingShares).length) return response.data.ocs.data