Page MenuHomePhabricator
Differential D11303

Boilerplate for web and desktop to look for keyserverID during notifications session creation and notification decryption
ClosedPublic
Actions

Authored by marcin on Mar 11 2024, 9:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 9, 12:38 AM
Unknown Object (File)
Tue, Apr 8, 10:02 PM
Unknown Object (File)
Tue, Apr 8, 10:01 PM
Unknown Object (File)
Sat, Mar 29, 7:15 AM
Unknown Object (File)
Sat, Mar 29, 6:49 AM
Unknown Object (File)
Mon, Mar 24, 2:42 PM
Unknown Object (File)
Mar 13 2025, 6:14 AM
Unknown Object (File)
Mar 11 2025, 9:22 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
michal
Commits
rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications…
Summary

This differential updates web and desktop code so that:

  1. The keyserverID is an argument to methods that create session.
  2. The keyserverID is an argument of methods that decrypt notifications on desktop.

Actually using this argument will be introduced in next diff.

Test Plan
  1. Flow
  2. Test that notifs aren't broken on web and desktop

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Mar 11 2024, 9:25 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 11 2024, 9:25 AM
marcin added a parent revision: D11302: Send keyserverID in web and desktop notifications.Mar 11 2024, 9:33 AM
marcin added a child revision: D11304: Refactor notifications sessions storage to support multiple keyservers on web and desktop.
marcin updated this revision to Diff 38002.Mar 11 2024, 9:36 AM

Rebase to update commit message

marcin edited the summary of this revision. (Show Details)Mar 11 2024, 9:37 AM
Harbormaster completed remote builds in B27450: Diff 37999.Mar 11 2024, 9:40 AM
Harbormaster completed remote builds in B27453: Diff 38002.Mar 11 2024, 9:51 AM
marcin requested review of this revision.Mar 11 2024, 9:51 AM
marcin planned changes to this revision.Mar 12 2024, 8:29 AM

Diff will change significantly due to: https://linear.app/comm/issue/ENG-7194/we-introduced-a-regression-in-macos-notifications#comment-8d85367e

michal added inline comments.Mar 12 2024, 8:31 AM
desktop/flow-typed/npm/electron_v22.0.0.js
339–343

Sorry, I should have caught this when adding (single-keyserver) encrypted notifications to desktop -> types in flow-typed are types for specific libraries and shouldn't be mixed with our types (in this case the keyserverID and encryptedPayload fields). Could we (potentially in a different diff) remove them and only keep the +[string]: mixed and add a check in JS code that checks if these fields exists.

marcin updated this revision to Diff 38052.Mar 14 2024, 4:14 AM
  1. Approach changed - we are moving with this proejct as it is and will fix desktop regression as a separate project.
  2. Fix types in electron_v22.0.0.js
Harbormaster completed remote builds in B27495: Diff 38052.Mar 14 2024, 4:29 AM
michal requested changes to this revision.Mar 14 2024, 6:30 AM

In general the code looks good but there is one major problem: if someone doesn't update their desktop app they can still run a newer version of the web app.

This diff breaks the API between webapp and electron code by adding changing the arguments to the onEncryptedNotification (adding the keyserverID as the last argument would probably be fine). But the bigger problem is handling of this edge case with encrypting notifications...

desktop/src/push-notifications.js
146 ↗(On Diff #38052)

At this point the encryptedPayload must be a string. So I don't think the showNewNotification(userInfo, handleClick); will ever run. You should probably check for if (!userInfo.encryptedPayload) first.

156–157 ↗(On Diff #38052)

Don't we need a typeof keyserverID !== 'string' || typeof encryptedPayload !== 'string' check here too?

This revision now requires changes to proceed.Mar 14 2024, 6:30 AM
marcin added inline comments.Mar 15 2024, 2:26 AM
desktop/src/push-notifications.js
156–157 ↗(On Diff #38052)

True - my oversight.

marcin updated this revision to Diff 38096.Mar 15 2024, 6:21 AM
  1. Address JS-nits
  2. Fixx potential issues realted to desktop-web code version discrepancy
Harbormaster completed remote builds in B27528: Diff 38096.Mar 15 2024, 6:37 AM
michal accepted this revision.Mar 18 2024, 4:15 AM
This revision is now accepted and ready to land.Mar 18 2024, 4:15 AM
marcin updated this revision to Diff 38222.Mar 20 2024, 4:17 AM

Rebase before landing

Harbormaster completed remote builds in B27624: Diff 38222.Mar 20 2024, 4:32 AM
Closed by commit rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications… (authored by marcin). · Explain WhyMar 20 2024, 5:37 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications….

Revision Contents
Changeset List

PathSize
desktop/
flow-typed/
npm/
6 lines
src/
6 lines
15 lines
lib/
types/
1 line
web/
account/
2 lines
push-notif/
7 lines
9 lines

Diff 38002

View Options

desktop/flow-typed/npm/electron_v22.0.0.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines
View Options

desktop/src/main.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines
View Options

desktop/src/push-notifications.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines
View Options

lib/types/electron-types.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines
View Options

web/account/account-hooks.js

Loading...
View Options

web/push-notif/notif-crypto-utils.js

Loading...
View Options

web/push-notif/push-notifs-handler.js

Loading...