From 3299b8f3237e0f1acecd22a3d7d3aedd11fda45c Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Mon, 7 Aug 2023 16:12:57 -0700 Subject: [PATCH] Remove obsolete capabilities, improve routine profile fetch --- ts/background.ts | 6 - ts/models/conversations.ts | 2 - ts/routineProfileRefresh.ts | 71 ++----- .../util/isConversationUnregistered_test.ts | 29 ++- .../routineProfileRefresh_test.ts | 179 ++++++++---------- ts/textsecure/WebAPI.ts | 18 -- ts/util/getRecipients.ts | 7 +- ts/util/handleRetry.ts | 9 +- ts/util/isConversationUnregistered.ts | 24 ++- ts/util/sendEditedMessage.ts | 4 +- ts/util/sendToGroup.ts | 10 - 11 files changed, 147 insertions(+), 212 deletions(-) diff --git a/ts/background.ts b/ts/background.ts index b778bbacf..1d766e5d0 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -1850,12 +1850,6 @@ export async function startApp(): Promise { // Note: we always have to register our capabilities all at once, so we do this // after connect on every startup await server.registerCapabilities({ - announcementGroup: true, - giftBadges: true, - 'gv2-3': true, - senderKey: true, - changeNumber: true, - stories: true, pni: isPnpEnabled(), }); } catch (error) { diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index 91f7284b0..ecef55762 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -3633,7 +3633,6 @@ export class ConversationModel extends window.Backbone getRecipients({ includePendingMembers, extraConversationsForSend, - isStoryReply = false, }: { includePendingMembers?: boolean; extraConversationsForSend?: ReadonlyArray; @@ -3642,7 +3641,6 @@ export class ConversationModel extends window.Backbone return getRecipients(this.attributes, { includePendingMembers, extraConversationsForSend, - isStoryReply, }); } diff --git a/ts/routineProfileRefresh.ts b/ts/routineProfileRefresh.ts index 13860b7a5..8bef2c99d 100644 --- a/ts/routineProfileRefresh.ts +++ b/ts/routineProfileRefresh.ts @@ -7,7 +7,6 @@ import PQueue from 'p-queue'; import * as log from './logging/log'; import { assertDev } from './util/assert'; import { sleep } from './util/sleep'; -import { missingCaseError } from './util/missingCaseError'; import { isNormalNumber } from './util/isNormalNumber'; import { take } from './util/iterables'; import type { ConversationModel } from './models/conversations'; @@ -15,13 +14,13 @@ import type { StorageInterface } from './types/Storage.d'; import * as Errors from './types/errors'; import { getProfile } from './util/getProfile'; import { drop } from './util/drop'; -import { MINUTE, HOUR, DAY, WEEK, MONTH } from './util/durations'; +import { MINUTE, HOUR, DAY, WEEK } from './util/durations'; +import { isDirectConversation } from './util/whatTypeOfConversation'; const STORAGE_KEY = 'lastAttemptedToRefreshProfilesAt'; -const MAX_AGE_TO_BE_CONSIDERED_ACTIVE = MONTH; -const MAX_AGE_TO_BE_CONSIDERED_RECENTLY_REFRESHED = DAY; +const MAX_AGE_TO_BE_CONSIDERED_RECENTLY_REFRESHED = 3 * DAY; const MAX_CONVERSATIONS_TO_REFRESH = 50; -const MIN_ELAPSED_DURATION_TO_REFRESH_AGAIN = 12 * HOUR; +const MIN_ELAPSED_DURATION_TO_REFRESH_AGAIN = HOUR; const MIN_REFRESH_DELAY = MINUTE; let idCounter = 1; @@ -200,60 +199,28 @@ function* getFilteredConversations( conversations: ReadonlyArray, ourConversationId: string ): Iterable { - const sorted = sortBy(conversations, c => c.get('active_at')); - - const conversationIdsSeen = new Set([ourConversationId]); + const filtered = conversations.filter( + c => + isDirectConversation(c.attributes) && + !c.isUnregisteredAndStale() && + c.get('uuid') + ); + const sorted = sortBy(filtered, c => c.get('profileLastFetchedAt') || 0); for (const conversation of sorted) { - const type = conversation.get('type'); - switch (type) { - case 'private': - if ( - conversation.hasProfileKeyCredentialExpired() && - (conversation.id === ourConversationId || - !conversationIdsSeen.has(conversation.id)) - ) { - conversationIdsSeen.add(conversation.id); - yield conversation; - break; - } + if (conversation.id === ourConversationId) { + if (conversation.hasProfileKeyCredentialExpired()) { + yield conversation; + } + continue; + } - if ( - !conversationIdsSeen.has(conversation.id) && - isConversationActive(conversation) && - !hasRefreshedProfileRecently(conversation) - ) { - conversationIdsSeen.add(conversation.id); - yield conversation; - } - break; - case 'group': - for (const member of conversation.getMembers()) { - if ( - !conversationIdsSeen.has(member.id) && - !hasRefreshedProfileRecently(member) - ) { - conversationIdsSeen.add(member.id); - yield member; - } - } - break; - default: - throw missingCaseError(type); + if (!hasRefreshedProfileRecently(conversation)) { + yield conversation; } } } -function isConversationActive( - conversation: Readonly -): boolean { - const activeAt = conversation.get('active_at'); - return ( - isNormalNumber(activeAt) && - activeAt + MAX_AGE_TO_BE_CONSIDERED_ACTIVE > Date.now() - ); -} - function hasRefreshedProfileRecently( conversation: Readonly ): boolean { diff --git a/ts/test-both/util/isConversationUnregistered_test.ts b/ts/test-both/util/isConversationUnregistered_test.ts index 64658cd37..7af084e69 100644 --- a/ts/test-both/util/isConversationUnregistered_test.ts +++ b/ts/test-both/util/isConversationUnregistered_test.ts @@ -8,20 +8,36 @@ import { isConversationUnregistered } from '../../util/isConversationUnregistere describe('isConversationUnregistered', () => { it('returns false if passed an undefined discoveredUnregisteredAt', () => { - assert.isFalse(isConversationUnregistered({})); + assert.isFalse(isConversationUnregistered({ uuid: 'uuid' })); assert.isFalse( - isConversationUnregistered({ discoveredUnregisteredAt: undefined }) + isConversationUnregistered({ + uuid: 'uuid', + discoveredUnregisteredAt: undefined, + }) + ); + }); + + it('returns true if uuid is falsey', () => { + assert.isTrue( + isConversationUnregistered({ + uuid: undefined, + discoveredUnregisteredAt: Date.now() + 123, + }) ); }); it('returns true if passed a time fewer than 6 hours ago', () => { assert.isTrue( - isConversationUnregistered({ discoveredUnregisteredAt: Date.now() }) + isConversationUnregistered({ + uuid: 'uuid', + discoveredUnregisteredAt: Date.now(), + }) ); const fiveHours = 1000 * 60 * 60 * 5; assert.isTrue( isConversationUnregistered({ + uuid: 'uuid', discoveredUnregisteredAt: Date.now() - fiveHours, }) ); @@ -29,19 +45,24 @@ describe('isConversationUnregistered', () => { it('returns true if passed a time in the future', () => { assert.isTrue( - isConversationUnregistered({ discoveredUnregisteredAt: Date.now() + 123 }) + isConversationUnregistered({ + uuid: 'uuid', + discoveredUnregisteredAt: Date.now() + 123, + }) ); }); it('returns false if passed a time more than 6 hours ago', () => { assert.isFalse( isConversationUnregistered({ + uuid: 'uuid', discoveredUnregisteredAt: Date.now() - 6 * durations.HOUR - durations.MINUTE, }) ); assert.isFalse( isConversationUnregistered({ + uuid: 'uuid', discoveredUnregisteredAt: new Date(1999, 3, 20).getTime(), }) ); diff --git a/ts/test-electron/routineProfileRefresh_test.ts b/ts/test-electron/routineProfileRefresh_test.ts index 5b83aa76b..a290f6e15 100644 --- a/ts/test-electron/routineProfileRefresh_test.ts +++ b/ts/test-electron/routineProfileRefresh_test.ts @@ -6,7 +6,7 @@ import { times } from 'lodash'; import { ConversationModel } from '../models/conversations'; import type { ConversationAttributesType } from '../model-types.d'; import { UUID } from '../types/UUID'; -import { DAY } from '../util/durations'; +import { DAY, HOUR, MINUTE, MONTH } from '../util/durations'; import { routineProfileRefresh } from '../routineProfileRefresh'; @@ -79,10 +79,10 @@ describe('routineProfileRefresh', () => { }; } - it('does nothing when the last refresh time is less than 12 hours ago', async () => { + it('does nothing when the last refresh time is less one hour', async () => { const conversation1 = makeConversation(); const conversation2 = makeConversation(); - const storage = makeStorage(Date.now() - 1234); + const storage = makeStorage(Date.now() - 47 * MINUTE); await routineProfileRefresh({ allConversations: [conversation1, conversation2], @@ -120,15 +120,17 @@ describe('routineProfileRefresh', () => { ); }); - it("skips conversations that haven't been active in 30 days", async () => { - const recentlyActive = makeConversation(); - const inactive = makeConversation({ - active_at: Date.now() - 31 * 24 * 60 * 60 * 1000, + it('skips unregistered conversations and those fetched in the last three days', async () => { + const normal = makeConversation(); + const recentlyFetched = makeConversation({ + profileLastFetchedAt: Date.now() - DAY * 2 - HOUR * 3, + }); + const unregisteredAndStale = makeConversation({ + firstUnregisteredAt: Date.now() - 2 * MONTH, }); - const neverActive = makeConversation({ active_at: undefined }); await routineProfileRefresh({ - allConversations: [recentlyActive, inactive, neverActive], + allConversations: [normal, recentlyFetched, unregisteredAndStale], ourConversationId: UUID.generate().toString(), storage: makeStorage(), getProfileFn, @@ -138,18 +140,18 @@ describe('routineProfileRefresh', () => { sinon.assert.calledOnce(getProfileFn); sinon.assert.calledWith( getProfileFn, - recentlyActive.get('uuid'), - recentlyActive.get('e164') + normal.get('uuid'), + normal.get('e164') ); sinon.assert.neverCalledWith( getProfileFn, - inactive.get('uuid'), - inactive.get('e164') + recentlyFetched.get('uuid'), + recentlyFetched.get('e164') ); sinon.assert.neverCalledWith( getProfileFn, - neverActive.get('uuid'), - neverActive.get('e164') + unregisteredAndStale.get('uuid'), + unregisteredAndStale.get('e164') ); }); @@ -169,21 +171,56 @@ describe('routineProfileRefresh', () => { sinon.assert.neverCalledWith(getProfileFn, me.get('uuid'), me.get('e164')); }); - it('skips conversations that were refreshed in the last hour', async () => { - const neverRefreshed = makeConversation(); - const recentlyFetched = makeConversation({ - profileLastFetchedAt: Date.now() - 59 * 60 * 1000, + it('includes your own conversation if profileKeyCredential is expired', async () => { + const notMe = makeConversation(); + const me = makeConversation({ + profileKey: 'fakeProfileKey', + profileKeyCredential: undefined, + profileKeyCredentialExpiration: undefined, }); await routineProfileRefresh({ - allConversations: [neverRefreshed, recentlyFetched], + allConversations: [notMe, me], + ourConversationId: me.id, + storage: makeStorage(), + getProfileFn, + id: 1, + }); + + sinon.assert.calledWith(getProfileFn, notMe.get('uuid'), notMe.get('e164')); + sinon.assert.calledWith(getProfileFn, me.get('uuid'), me.get('e164')); + }); + + it('skips conversations that were refreshed in last three days', async () => { + const neverRefreshed = makeConversation(); + const refreshedToday = makeConversation({ + profileLastFetchedAt: Date.now() - HOUR * 5, + }); + const refreshedYesterday = makeConversation({ + profileLastFetchedAt: Date.now() - DAY, + }); + const refreshedTwoDaysAgo = makeConversation({ + profileLastFetchedAt: Date.now() - DAY * 2, + }); + const refreshedThreeDaysAgo = makeConversation({ + profileLastFetchedAt: Date.now() - DAY * 3 - 1, + }); + + await routineProfileRefresh({ + allConversations: [ + neverRefreshed, + refreshedToday, + refreshedYesterday, + refreshedTwoDaysAgo, + refreshedThreeDaysAgo, + ], ourConversationId: UUID.generate().toString(), storage: makeStorage(), getProfileFn, id: 1, }); - sinon.assert.calledOnce(getProfileFn); + sinon.assert.calledTwice(getProfileFn); sinon.assert.calledWith( getProfileFn, neverRefreshed.get('uuid'), @@ -191,105 +228,55 @@ describe('routineProfileRefresh', () => { ); sinon.assert.neverCalledWith( getProfileFn, - recentlyFetched.get('uuid'), - recentlyFetched.get('e164') - ); - }); - - it('"digs into" the members of an active group', async () => { - const privateConversation = makeConversation(); - - const recentlyActiveGroupMember = makeConversation(); - const inactiveGroupMember = makeConversation({ - active_at: Date.now() - 31 * 24 * 60 * 60 * 1000, - }); - const memberWhoHasRecentlyRefreshed = makeConversation({ - profileLastFetchedAt: Date.now() - 59 * 60 * 1000, - }); - - const groupConversation = makeGroup([ - recentlyActiveGroupMember, - inactiveGroupMember, - memberWhoHasRecentlyRefreshed, - ]); - - await routineProfileRefresh({ - allConversations: [ - privateConversation, - recentlyActiveGroupMember, - inactiveGroupMember, - memberWhoHasRecentlyRefreshed, - groupConversation, - ], - ourConversationId: UUID.generate().toString(), - storage: makeStorage(), - getProfileFn, - id: 1, - }); - - sinon.assert.calledWith( - getProfileFn, - privateConversation.get('uuid'), - privateConversation.get('e164') - ); - sinon.assert.calledWith( - getProfileFn, - recentlyActiveGroupMember.get('uuid'), - recentlyActiveGroupMember.get('e164') - ); - sinon.assert.calledWith( - getProfileFn, - inactiveGroupMember.get('uuid'), - inactiveGroupMember.get('e164') + refreshedToday.get('uuid'), + refreshedToday.get('e164') ); sinon.assert.neverCalledWith( getProfileFn, - memberWhoHasRecentlyRefreshed.get('uuid'), - memberWhoHasRecentlyRefreshed.get('e164') + refreshedYesterday.get('uuid'), + refreshedYesterday.get('e164') ); sinon.assert.neverCalledWith( getProfileFn, - groupConversation.get('uuid'), - groupConversation.get('e164') + refreshedTwoDaysAgo.get('uuid'), + refreshedTwoDaysAgo.get('e164') + ); + sinon.assert.calledWith( + getProfileFn, + refreshedThreeDaysAgo.get('uuid'), + refreshedThreeDaysAgo.get('e164') ); }); - it('only refreshes profiles for the 50 most recently active direct conversations', async () => { + it('only refreshes profiles for the 50 conversations with the oldest profileLastFetchedAt', async () => { const me = makeConversation(); - const activeConversations = times(40, () => makeConversation()); - - const inactiveGroupMembers = times(10, () => + const normalConversations = times(25, () => makeConversation()); + const neverFetched = times(10, () => makeConversation({ - active_at: Date.now() - 999 * 24 * 60 * 60 * 1000, + profileLastFetchedAt: undefined, + }) + ); + const unregisteredUsers = times(10, () => + makeConversation({ + firstUnregisteredAt: Date.now() - MONTH * 2, }) ); - const recentlyActiveGroup = makeGroup(inactiveGroupMembers); const shouldNotBeIncluded = [ // Recently-active groups with no other members makeGroup([]), makeGroup([me]), - // Old direct conversations - ...times(3, () => - makeConversation({ - active_at: Date.now() - 365 * 24 * 60 * 60 * 1000, - }) - ), - // Old groups - ...times(3, () => makeGroup(inactiveGroupMembers)), + ...unregisteredUsers, ]; await routineProfileRefresh({ allConversations: [ me, - ...activeConversations, - - recentlyActiveGroup, - ...inactiveGroupMembers, - - ...shouldNotBeIncluded, + ...unregisteredUsers, + ...normalConversations, + ...neverFetched, ], ourConversationId: me.id, storage: makeStorage(), @@ -297,7 +284,7 @@ describe('routineProfileRefresh', () => { id: 1, }); - [...activeConversations, ...inactiveGroupMembers].forEach(conversation => { + [...normalConversations, ...neverFetched].forEach(conversation => { sinon.assert.calledWith( getProfileFn, conversation.get('uuid'), diff --git a/ts/textsecure/WebAPI.ts b/ts/textsecure/WebAPI.ts index 084776be7..0233e320e 100644 --- a/ts/textsecure/WebAPI.ts +++ b/ts/textsecure/WebAPI.ts @@ -623,21 +623,9 @@ export type WebAPIConnectType = { }; export type CapabilitiesType = { - announcementGroup: boolean; - giftBadges: boolean; - senderKey: boolean; - changeNumber: boolean; - stories: boolean; pni: boolean; }; export type CapabilitiesUploadType = { - announcementGroup: true; - giftBadges: true; - 'gv2-3': true; - senderKey: true; - changeNumber: true; - stories: true; - // true in staging, false in production pni: boolean; }; @@ -2073,12 +2061,6 @@ export function initialize({ accessKey, }: ConfirmCodeOptionsType) { const capabilities: CapabilitiesUploadType = { - announcementGroup: true, - giftBadges: true, - 'gv2-3': true, - senderKey: true, - changeNumber: true, - stories: true, pni: isPnpEnabled(), }; diff --git a/ts/util/getRecipients.ts b/ts/util/getRecipients.ts index 3b4e97be5..3f18354e0 100644 --- a/ts/util/getRecipients.ts +++ b/ts/util/getRecipients.ts @@ -15,7 +15,6 @@ export function getRecipients( { includePendingMembers, extraConversationsForSend, - isStoryReply = false, }: { includePendingMembers?: boolean; extraConversationsForSend?: ReadonlyArray; @@ -27,14 +26,10 @@ export function getRecipients( return [getSendTarget(conversationAttributes)!]; } - let members = getConversationMembers(conversationAttributes, { + const members = getConversationMembers(conversationAttributes, { includePendingMembers, }); - if (isStoryReply) { - members = members.filter(({ capabilities }) => capabilities?.stories); - } - // There are cases where we need to send to someone we just removed from the group, to // let them know that we removed them. In that case, we need to send to more than // are currently in the group. diff --git a/ts/util/handleRetry.ts b/ts/util/handleRetry.ts index 088bdd729..062f315d6 100644 --- a/ts/util/handleRetry.ts +++ b/ts/util/handleRetry.ts @@ -246,17 +246,10 @@ export async function onDecryptionError( senderUuid, 'private' ); - if (!conversation.get('capabilities')?.senderKey) { - await conversation.getProfiles(); - } - const name = conversation.getTitle(); maybeShowDecryptionToast(logId, name, senderDevice); - if ( - conversation.get('capabilities')?.senderKey && - RemoteConfig.isEnabled('desktop.senderKey.retry') - ) { + if (RemoteConfig.isEnabled('desktop.senderKey.retry')) { await requestResend(decryptionError); } else { await startAutomaticSessionReset(decryptionError); diff --git a/ts/util/isConversationUnregistered.ts b/ts/util/isConversationUnregistered.ts index d9e04ce43..1ee051a74 100644 --- a/ts/util/isConversationUnregistered.ts +++ b/ts/util/isConversationUnregistered.ts @@ -7,23 +7,33 @@ import { HOUR, MONTH } from './durations'; const SIX_HOURS = 6 * HOUR; export function isConversationEverUnregistered({ + uuid, discoveredUnregisteredAt, -}: Readonly<{ discoveredUnregisteredAt?: number }>): boolean { - return discoveredUnregisteredAt !== undefined; +}: Readonly<{ uuid?: string; discoveredUnregisteredAt?: number }>): boolean { + return !uuid || discoveredUnregisteredAt !== undefined; } export function isConversationUnregistered({ + uuid, discoveredUnregisteredAt, -}: Readonly<{ discoveredUnregisteredAt?: number }>): boolean { - return Boolean( - discoveredUnregisteredAt && - isMoreRecentThan(discoveredUnregisteredAt, SIX_HOURS) +}: Readonly<{ uuid?: string; discoveredUnregisteredAt?: number }>): boolean { + return ( + !uuid || + Boolean( + discoveredUnregisteredAt && + isMoreRecentThan(discoveredUnregisteredAt, SIX_HOURS) + ) ); } export function isConversationUnregisteredAndStale({ + uuid, firstUnregisteredAt, -}: Readonly<{ firstUnregisteredAt?: number }>): boolean { +}: Readonly<{ uuid?: string; firstUnregisteredAt?: number }>): boolean { + if (!uuid) { + return true; + } + return Boolean( firstUnregisteredAt && isOlderThan(firstUnregisteredAt, MONTH) ); diff --git a/ts/util/sendEditedMessage.ts b/ts/util/sendEditedMessage.ts index 99b5d8e00..ec918b5a2 100644 --- a/ts/util/sendEditedMessage.ts +++ b/ts/util/sendEditedMessage.ts @@ -99,9 +99,7 @@ export async function sendEditedMessage( const fromId = ourConversation.id; const recipientMaybeConversations = map( - conversation.getRecipients({ - isStoryReply: false, - }), + conversation.getRecipients(), identifier => window.ConversationController.get(identifier) ); const recipientConversations = filter(recipientMaybeConversations, isNotNil); diff --git a/ts/util/sendToGroup.ts b/ts/util/sendToGroup.ts index 73dee6038..a630779c8 100644 --- a/ts/util/sendToGroup.ts +++ b/ts/util/sendToGroup.ts @@ -198,14 +198,9 @@ export async function sendContentMessageToGroup({ 'sendContentMessageToGroup: textsecure.messaging not available!' ); - const ourConversationId = - window.ConversationController.getOurConversationIdOrThrow(); - const ourConversation = window.ConversationController.get(ourConversationId); - if ( isEnabled('desktop.sendSenderKey3') && isEnabled('desktop.senderKey.send') && - ourConversation?.get('capabilities')?.senderKey && sendTarget.isValid() ) { try { @@ -1148,11 +1143,6 @@ function isValidSenderKeyRecipient( return false; } - const capabilities = memberConversation.get('capabilities'); - if (!capabilities?.senderKey) { - return false; - } - if (!getAccessKey(memberConversation.attributes, { story })) { return false; }