Refactor encryption code to use bot-sdk #369

Half-Shot · 2021-11-03T10:53:10Z

This code refactors our encryption broker component and intents to generally handle encrypted events better. The changes are:

Use the bot-sdk for encryption, rather than the js-sdk
Drop support for ephemeral events via /sync, preferring the AS route.
Refactors the session creation code within Intent, hopefully reducing the chance of a race.
Improves the example encryption app.

- Not handle ephemeral, those come down the AS path - Use the bot sdk

jaller94 · 2021-11-04T08:13:26Z

src/components/intent.ts

+        if (!session) {
+            // No session in the store, attempt a login.
+            log.debug("ensureRegistered: Attempting encrypted login");
+            // Login as the user


Login is the noun.

Suggested change

// Login as the user

// Log in as the user

jaller94 · 2021-11-04T08:16:07Z

src/components/intent.ts

-    private readyPromise?: Promise<unknown>;
+    private encryptionReadyPromise?: Promise<void>;
+
+    // A client that talks directly to the homeserver, bypassing pan.


To reduce the knowledge one needs to have to understand this comment, I'd recommend not abbreviating pantalaimon.

Suggested change

// A client that talks directly to the homeserver, bypassing pan.

// A client that talks directly to the homeserver, bypassing pantalaimon.

jaller94 · 2021-11-04T08:16:46Z

src/components/intent.ts

@@ -1180,51 +1185,81 @@ export class Intent {
            }
        }

+        // NOTES:
+        // Still seeing errors about invalid access tokens.
+        // Still seeing the server attempt to send stuff to pan without sync being enabled.


Suggested change

// Still seeing the server attempt to send stuff to pan without sync being enabled.

// Still seeing the server attempt to send stuff to pantalaimon without sync being enabled.

jaller94 · 2021-11-04T08:17:11Z

src/components/encryption.ts

-        if (!event.event.decrypted) {
+    private onSyncEvent(roomId: string, event: PanWeakEvent): void {
+        log.info("BLARGH", roomId, event);
+        if (!event.decrypted) {
            // We only care about encrypted events, and pan appends a decrypted key to each event.


Suggested change

// We only care about encrypted events, and pan appends a decrypted key to each event.

// We only care about encrypted events, and pantalaimon appends a decrypted key to each event.

jaller94 · 2021-11-04T08:17:44Z

src/components/encryption.ts

-    private onSyncEvent(event: any) {
-        if (!event.event.decrypted) {
+    private onSyncEvent(roomId: string, event: PanWeakEvent): void {
+        log.info("BLARGH", roomId, event);


Suggested change

log.info("BLARGH", roomId, event);

or

Suggested change

log.info("BLARGH", roomId, event);

log.debug("onSyncEvent", roomId, event);

jaller94 · 2021-11-04T08:21:01Z

src/components/encryption.ts

            // We only care about encrypted events, and pan appends a decrypted key to each event.
            return;
        }
-        if (!this.eventsPendingSync.has(event.getId())) {
+        // TODO: Do we need this?


Just pointing this out: You've left a TODO here.

jaller94 · 2021-11-04T08:27:52Z

src/components/intent.ts

+        if (!this.encryption) {
+            throw Error('Cannot call getEncryptedSession without enabling encryption');
+        }
+        // We've not got a session so let's see if the store has one.


Suggested change

// We've not got a session so let's see if the store has one.

// We've got no session so let's see if the store has one.

jaller94 · 2021-11-04T08:29:31Z

src/components/intent.ts

-                log.debug("ensureRegistered: failed to ready", ex);
-                // Failed to ready up - fall through and try again.
-            }
+        // Past this point, we're an encryption enabled Intent.


Suggested change

// Past this point, we're an encryption enabled Intent.

// Past this point, we're an encryption-enabled Intent.

jaller94 · 2021-11-04T08:34:15Z

src/bridge.ts

        };
        clientIntentOpts.registered = this.membershipCache.isUserRegistered(userId);
        const encryptionOpts = this.opts.bridgeEncryption;
        if (encryptionOpts) {
            clientIntentOpts.encryption = {
                sessionPromise: encryptionOpts.store.getStoredSession(userId),
+                origianlHomeserver: this.opts.homeserverUrl,


typo that hopefully still can be fixed in the entire repo.

Suggested change

origianlHomeserver: this.opts.homeserverUrl,

originalHomeserver: this.opts.homeserverUrl,

Half-Shot · 2021-11-04T14:31:40Z

I had an epiphany and decided to refactor the encryption code into a separate subclass so it's easier to understand (and less if/else-ing)

src/components/membership-cache.ts

src/components/encrypted-intent.ts

tadzik · 2021-11-04T15:39:12Z

src/components/encrypted-intent.ts

+        };
+    }
+
+    public async ensureRegistered(forceRegister = false): Promise<"registered=true"|undefined> {


What's the reason for it returning this awkward string rather than a true (or just void, since it rejects on failure anyway?)

It's a thing that the old old sdk did and we've just carried it forward. I'm not sure if anyone actually expects it..

src/components/encryption.ts

src/components/encrypted-intent.ts

Half-Shot added 13 commits November 3, 2021 10:50

Add quick-lru

722a48c

If enabling encryption, always route requests via pan

eac28c3

Refactor EncryptedEventBroker to:

092079b

- Not handle ephemeral, those come down the AS path - Use the bot sdk

Make ensure registered check the token

f1e441d

Improve signatures on MembershipCache

d25bb95

Tidy up onEventSend

d74a1d0

Tidy up example

c7263d3

Update encryption packages

53ff954

Switch encryption example to yarn

42ebee9

Update express dependency to reduce awful errors

ccfbc3b

Add proper filter setting code

44f8606

Refactor session grabbing code

9839357

changelog

d20d9dd

Half-Shot requested a review from a team November 3, 2021 12:02

add a note about ephemeral events

d2ebc7a

Half-Shot changed the title ~~Refactor encryption code to use pantaliamon~~ Refactor encryption code to use bot-sdk Nov 3, 2021

Half-Shot added 7 commits November 3, 2021 13:30

Avoid races

1d9020a

Be more careful about when we engage sync

bd3bb65

Add origianlHomeserver

e71a61f

more debugging

112d2f2

impersonate properly

a89cf3a

impersonateUserId

9b8ec7f

Properly wrap the send

8cb1bce

jaller94 suggested changes Nov 4, 2021

View reviewed changes

Half-Shot added 6 commits November 4, 2021 13:49

Tidy up some code

a838451

s/pan/pantalaimon

1c3cbf5

Remove todo

40e308b

Fixup room_id

c149bc4

Encryption enabled intent

7861426

Refactor out encryption intent code to separate subclass

8242f6f

Half-Shot force-pushed the hs/encryption-refactore branch from 70cb5cc to 8242f6f Compare November 4, 2021 14:22

Half-Shot added 2 commits November 4, 2021 14:22

Reformat changelog

53eb846

Depend on intent

f412d6d

Half-Shot requested a review from jaller94 November 4, 2021 14:31

Tweak broker to be less memory intensive.

2937c1f

tadzik reviewed Nov 4, 2021

View reviewed changes

Tweaks to code

159b2b4

Half-Shot requested a review from tadzik November 17, 2021 17:06

tadzik approved these changes Nov 18, 2021

View reviewed changes

Half-Shot merged commit 91f709e into develop Nov 18, 2021

Half-Shot deleted the hs/encryption-refactore branch May 2, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor encryption code to use bot-sdk #369

Refactor encryption code to use bot-sdk #369

Half-Shot commented Nov 3, 2021 •

edited

Loading

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

jaller94 Nov 4, 2021

Half-Shot commented Nov 4, 2021

tadzik Nov 4, 2021

Half-Shot Nov 10, 2021

	// A client that talks directly to the homeserver, bypassing pan.
	// A client that talks directly to the homeserver, bypassing pantalaimon.

	// Still seeing the server attempt to send stuff to pan without sync being enabled.
	// Still seeing the server attempt to send stuff to pantalaimon without sync being enabled.

	// We only care about encrypted events, and pan appends a decrypted key to each event.
	// We only care about encrypted events, and pantalaimon appends a decrypted key to each event.

	log.info("BLARGH", roomId, event);
	log.debug("onSyncEvent", roomId, event);

	// We've not got a session so let's see if the store has one.
	// We've got no session so let's see if the store has one.

	// Past this point, we're an encryption enabled Intent.
	// Past this point, we're an encryption-enabled Intent.

	origianlHomeserver: this.opts.homeserverUrl,
	originalHomeserver: this.opts.homeserverUrl,

Refactor encryption code to use bot-sdk #369

Refactor encryption code to use bot-sdk #369

Conversation

Half-Shot commented Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Half-Shot commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Half-Shot commented Nov 3, 2021 •

edited

Loading