From e566e2551230fd8a1c5ac3e95b605594251033a3 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Wed, 14 Aug 2024 09:33:20 +0200 Subject: [PATCH 1/4] fix(capabilities): move from token match to remoteServer match Signed-off-by: Maksim Sukharev --- src/services/CapabilitiesManager.ts | 61 ++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index 3324a607480..434968e92a3 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -10,14 +10,35 @@ 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, JoinRoomFullResponse } from '../types' +import type { Capabilities, Conversation, JoinRoomFullResponse } from '../types' type Config = Capabilities['spreed']['config'] -type RemoteCapabilities = Record> +type RemoteCapability = Capabilities & Partial<{ hash: string }> +type RemoteCapabilities = Record +type TokenMap = Record + +let remoteTokenMap: TokenMap = generateTokenMap() const localCapabilities: Capabilities = _getCapabilities() as Capabilities const remoteCapabilities: RemoteCapabilities = restoreRemoteCapabilities() +/** + * Generate new token map based on remoteCapabilities and cachedConversation + */ +function generateTokenMap() { + const tokenMap: TokenMap = {} + const storageValue = BrowserStorage.getItem('cachedConversations') + if (!storageValue?.length) { + return {} + } + const cachedConversations = JSON.parse(storageValue) as Conversation[] + cachedConversations.forEach(conversation => { + tokenMap[conversation.token] = conversation.remoteServer || null + }) + + return tokenMap +} + /** * Check whether the feature is presented (in case of federation - on both servers) * @param token conversation token @@ -25,12 +46,13 @@ const remoteCapabilities: RemoteCapabilities = restoreRemoteCapabilities() */ export function hasTalkFeature(token: string = 'local', feature: string): boolean { const hasLocalTalkFeature = localCapabilities?.spreed?.features?.includes(feature) ?? false + const remoteCapabilities = getRemoteCapability(token) if (localCapabilities?.spreed?.['features-local']?.includes(feature)) { return hasLocalTalkFeature - } else if (token === 'local' || !remoteCapabilities[token]) { + } else if (token === 'local' || !remoteCapabilities) { return hasLocalTalkFeature } else { - return hasLocalTalkFeature && (remoteCapabilities[token]?.spreed?.features?.includes(feature) ?? false) + return hasLocalTalkFeature && (remoteCapabilities?.spreed?.features?.includes(feature) ?? false) } } @@ -41,25 +63,46 @@ export function hasTalkFeature(token: string = 'local', feature: string): boolea * @param key2 second-level key (e.g. 'allowed') */ export function getTalkConfig(token: string = 'local', key1: keyof Config, key2: keyof Config[keyof Config]) { + const remoteCapabilities = getRemoteCapability(token) if (localCapabilities?.spreed?.['config-local']?.[key1]?.includes(key2)) { return localCapabilities?.spreed?.config?.[key1]?.[key2] - } else if (token === 'local' || !remoteCapabilities[token]) { + } else if (token === 'local' || !remoteCapabilities) { return localCapabilities?.spreed?.config?.[key1]?.[key2] } else { // TODO discuss handling remote config (respect remote only / both / minimal) - return remoteCapabilities[token]?.spreed?.config?.[key1]?.[key2] + return remoteCapabilities?.spreed?.config?.[key1]?.[key2] } } +/** + * Returns capability for specified token (if already matches from one of remote servers) + * @param token token of the conversation + */ +function getRemoteCapability(token: string): RemoteCapability | null { + if (remoteTokenMap[token] === undefined) { + // Unknown conversation, attempt to get remoteServer from cached conversations + remoteTokenMap = generateTokenMap() + } + + const remoteServer = remoteTokenMap[token] + if (!token || token === 'local' || !remoteServer) { + // Local or no conversation opened + return null + } + + return remoteCapabilities[remoteServer] ?? null +} + /** * Compares talk hash from remote instance and fetch new capabilities if it doesn't match * @param joinRoomResponse server response */ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullResponse): Promise { const token = joinRoomResponse.data.ocs.data.token + const remoteServer = joinRoomResponse.data.ocs.data.remoteServer as string // Check if remote capabilities have not changed since last check - if (joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] === remoteCapabilities[token]?.hash) { + if (joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] === remoteCapabilities[remoteServer]?.hash) { return } @@ -73,8 +116,8 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon return } - remoteCapabilities[token] = { spreed: response.data.ocs.data } - remoteCapabilities[token].hash = joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] + remoteCapabilities[remoteServer] = { spreed: response.data.ocs.data } + remoteCapabilities[remoteServer].hash = joinRoomResponse.headers['x-nextcloud-talk-proxy-hash'] BrowserStorage.setItem('remoteCapabilities', JSON.stringify(remoteCapabilities)) // As normal capabilities update, requires a reload to take effect From 3a20c249ef12b4e655b1d3753521c48580361d1d Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Wed, 14 Aug 2024 09:34:54 +0200 Subject: [PATCH 2/4] fix(capabilities): add migration step for previously stored capabilities Signed-off-by: Maksim Sukharev --- src/services/CapabilitiesManager.ts | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index 434968e92a3..eafdf2033ef 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -130,10 +130,25 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon * Restores capabilities from BrowserStorage */ function restoreRemoteCapabilities(): RemoteCapabilities { - const remoteCapabilities = BrowserStorage.getItem('remoteCapabilities') - if (!remoteCapabilities?.length) { + const storageValue = BrowserStorage.getItem('remoteCapabilities') + if (!storageValue) { return {} } + const remoteCapabilities = JSON.parse(storageValue) as RemoteCapabilities + + // Migration step for capabilities based on token + let hasMigrated = false + Object.keys(remoteCapabilities).forEach(key => { + const remoteServer = remoteTokenMap[key] + if (remoteServer) { + remoteCapabilities[remoteServer] = remoteCapabilities[key] + delete remoteCapabilities[key] + hasMigrated = true + } + }) + if (hasMigrated) { + BrowserStorage.setItem('remoteCapabilities', JSON.stringify(remoteCapabilities)) + } - return JSON.parse(remoteCapabilities) as RemoteCapabilities + return remoteCapabilities } From fd513e3b051446382945bce4dcafd65fc1d68000 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Wed, 14 Aug 2024 09:35:30 +0200 Subject: [PATCH 3/4] fix(capabilities): patch tokenMap and cache for new / unknown conversations Signed-off-by: Maksim Sukharev --- src/components/LeftSidebar/InvitationHandler.vue | 2 ++ src/services/CapabilitiesManager.ts | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/components/LeftSidebar/InvitationHandler.vue b/src/components/LeftSidebar/InvitationHandler.vue index f0438d24574..21b19b3767f 100644 --- a/src/components/LeftSidebar/InvitationHandler.vue +++ b/src/components/LeftSidebar/InvitationHandler.vue @@ -152,6 +152,8 @@ export default { const conversation = await this.federationStore.acceptShare(id) if (conversation?.token) { this.$store.dispatch('addConversation', conversation) + // TODO move cacheConversations to the store action + this.$store.dispatch('cacheConversations') } this.checkIfNoMoreInvitations() }, diff --git a/src/services/CapabilitiesManager.ts b/src/services/CapabilitiesManager.ts index eafdf2033ef..1813551b6e1 100644 --- a/src/services/CapabilitiesManager.ts +++ b/src/services/CapabilitiesManager.ts @@ -39,6 +39,16 @@ function generateTokenMap() { return tokenMap } +/** + * Patch token map with new / updated remote conversation + * @param conversation conversation object from join response + */ +function patchTokenMap(conversation: Conversation) { + if (conversation.remoteServer) { + remoteTokenMap[conversation.token] = conversation.remoteServer + } +} + /** * Check whether the feature is presented (in case of federation - on both servers) * @param token conversation token @@ -119,6 +129,7 @@ export async function setRemoteCapabilities(joinRoomResponse: JoinRoomFullRespon remoteCapabilities[remoteServer] = { spreed: response.data.ocs.data } remoteCapabilities[remoteServer].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, please reload the page'), { From ea8a85dcb5a307e05b7116d990cf767f9382c061 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Wed, 14 Aug 2024 09:35:56 +0200 Subject: [PATCH 4/4] fix(capabilities): add test coverage Signed-off-by: Maksim Sukharev --- src/__mocks__/capabilities.ts | 8 + .../__tests__/CapabilitiesManager.spec.js | 161 ++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 src/services/__tests__/CapabilitiesManager.spec.js diff --git a/src/__mocks__/capabilities.ts b/src/__mocks__/capabilities.ts index d7048b03016..7a557e314f0 100644 --- a/src/__mocks__/capabilities.ts +++ b/src/__mocks__/capabilities.ts @@ -79,7 +79,10 @@ export const mockedCapabilities: Capabilities = { 'silent-send-state', 'chat-read-last', 'federation-v1', + 'federation-v2', 'ban-v1', + 'chat-reference-id', + 'mention-permissions', ], 'features-local': [ 'favorites', @@ -157,3 +160,8 @@ export const mockedCapabilities: Capabilities = { version: '20.0.0-dev.0', } } + +export const mockedRemotes = { + 'https://nextcloud1.local': { ...mockedCapabilities, hash: 'abc123', tokens: ['TOKEN3FED1'] }, + 'https://nextcloud2.local': { ...mockedCapabilities, hash: 'def123', tokens: ['TOKEN5FED2'] }, +} diff --git a/src/services/__tests__/CapabilitiesManager.spec.js b/src/services/__tests__/CapabilitiesManager.spec.js new file mode 100644 index 00000000000..9277b92bfbb --- /dev/null +++ b/src/services/__tests__/CapabilitiesManager.spec.js @@ -0,0 +1,161 @@ +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +import { createPinia, setActivePinia } from 'pinia' + +import { mockedCapabilities, mockedRemotes } from '../../__mocks__/capabilities.ts' +import { useTalkHashStore } from '../../stores/talkHash.js' +import { generateOCSResponse } from '../../test-helpers.js' +import BrowserStorage from '../BrowserStorage.js' +import { + hasTalkFeature, + getTalkConfig, + setRemoteCapabilities, +} from '../CapabilitiesManager.ts' +import { getRemoteCapabilities } from '../federationService.ts' + +jest.mock('../BrowserStorage', () => ({ + getItem: jest.fn(key => { + const mockedConversations = [ + { token: 'TOKEN1', remoteServer: undefined }, + { token: 'TOKEN2', remoteServer: undefined }, + { token: 'TOKEN3FED1', remoteServer: 'https://nextcloud1.local' }, + { token: 'TOKEN4FED1', remoteServer: 'https://nextcloud1.local' }, + { token: 'TOKEN5FED2', remoteServer: 'https://nextcloud2.local' }, + { token: 'TOKEN6FED2', remoteServer: 'https://nextcloud2.local' }, + ] + + if (key === 'remoteCapabilities') { + return JSON.stringify(mockedRemotes) + } else if (key === 'cachedConversations') { + return JSON.stringify(mockedConversations) + } + return null + }), + setItem: jest.fn(), + removeItem: jest.fn(), +})) + +jest.mock('../federationService', () => ({ + getRemoteCapabilities: jest.fn(), +})) + +describe('CapabilitiesManager', () => { + let talkHashStore + + beforeEach(() => { + setActivePinia(createPinia()) + talkHashStore = useTalkHashStore() + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + describe('hasTalkFeature - local conversation', () => { + it('should return false if the feature is not in the capabilities', () => { + expect(hasTalkFeature('TOKEN1', 'never-existed')).toBeFalsy() + }) + + it('should return true if the feature is in the capabilities', () => { + expect(hasTalkFeature('TOKEN1', 'federation-v1')).toBeTruthy() + }) + + it('should return true if the feature is in the local capabilities', () => { + expect(hasTalkFeature('local', 'favorites')).toBeTruthy() + }) + + it('should return true if the feature is in the features-local list', () => { + expect(hasTalkFeature('TOKEN1', 'favorites')).toBeTruthy() + }) + }) + + describe('hasTalkFeature - remote conversation', () => { + it('should return false if the feature is not in the capabilities', () => { + expect(hasTalkFeature('TOKEN3FED1', 'never-existed')).toBeFalsy() + }) + + it('should return true if the feature is in the capabilities', () => { + expect(hasTalkFeature('TOKEN3FED1', 'federation-v1')).toBeTruthy() + }) + }) + + describe('getTalkConfig - local conversation', () => { + it('should return false if the feature is not in the capabilities', () => { + expect(getTalkConfig('TOKEN1', 'never', 'existed')).toBeFalsy() + }) + + it('should return true if the feature is in the capabilities', () => { + expect(getTalkConfig('TOKEN1', 'call', 'enabled')).toBeTruthy() + }) + + it('should return true if the feature is in the local capabilities', () => { + expect(getTalkConfig('local', 'call', 'enabled')).toBeTruthy() + }) + + it('should return true if the feature is in the features-local list', () => { + expect(getTalkConfig('TOKEN1', 'attachments', 'allowed')).toBeTruthy() + }) + }) + + describe('getTalkConfig - remote conversation', () => { + it('should return false if the feature is not in the capabilities', () => { + expect(getTalkConfig('TOKEN3FED1', 'never', 'existed')).toBeFalsy() + }) + + it('should return true if the feature is in the capabilities', () => { + expect(getTalkConfig('TOKEN3FED1', 'call', 'enabled')).toBeTruthy() + }) + }) + + describe('getRemoteCapability', () => { + it('should return true for known remoteServer and unknown token capabilities', () => { + expect(hasTalkFeature('TOKEN4FED1', 'ban-v1')).toBeTruthy() + }) + it('should try to regenerate tokenMap for unknown token', () => { + hasTalkFeature('TOKEN7FED1', 'ban-v1') + expect(BrowserStorage.getItem).toHaveBeenCalledTimes(1) // retry once + expect(BrowserStorage.getItem).toHaveBeenCalledWith('cachedConversations') + }) + }) + + describe('setRemoteCapability', () => { + const [remoteServer, remoteCapabilities] = Object.entries(mockedRemotes)[0] + const token = remoteCapabilities.tokens[0] + + it('should early return if proxy hash unchanged', async () => { + const joinRoomResponseMock = generateOCSResponse({ + headers: { 'x-nextcloud-talk-proxy-hash': remoteCapabilities.hash }, + payload: { token, remoteServer }, + }) + await setRemoteCapabilities(joinRoomResponseMock) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeUndefined() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(0) + }) + + it('should early return if no capabilities received from server', async () => { + const joinRoomResponseMock = generateOCSResponse({ + headers: { 'x-nextcloud-talk-proxy-hash': `${remoteCapabilities.hash}001` }, + payload: { token, remoteServer }, + }) + const responseMock = generateOCSResponse({ payload: [] }) + getRemoteCapabilities.mockReturnValue(responseMock) + await setRemoteCapabilities(joinRoomResponseMock) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(0) + }) + + 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 }) + getRemoteCapabilities.mockReturnValue(responseMock) + await setRemoteCapabilities(joinRoomResponseMock) + expect(talkHashStore.isNextcloudTalkProxyHashDirty[token]).toBeTruthy() + expect(BrowserStorage.setItem).toHaveBeenCalledTimes(1) + }) + }) +})