fix: Validate lockboxes on action payloads

package.json

@@ -24,10 +24,10 @@
     "lint": "xo",
     "linklocal": "node ./scripts/link-local.js",
     "unlinklocal": "node ./scripts/link-local.js --unlink && pnpm install",
-    "test": "vitest",
-    "test:all": "run-s build lint test:run test:cy test:pw",
-    "test:run": "vitest run",
-    "test:log": "cross-env DEBUG=localfirst*,automerge* DEBUG_COLORS=1 pnpm test run",
+    "test": "cross-env ALLOW_ADD_MEMBER_TEST=true vitest",
+    "test:all": "cross-env ALLOW_ADD_MEMBER_TEST=true run-s build lint test:run test:cy test:pw",
+    "test:run": "cross-env ALLOW_ADD_MEMBER_TEST=true vitest run",
+    "test:log": "cross-env ALLOW_ADD_MEMBER_TEST=true cross-env DEBUG=localfirst*,automerge* DEBUG_COLORS=1 pnpm test run",
     "test:cy:ui": "pnpm -F @localfirst/taco-chat test:cy:ui",
     "test:cy": "pnpm -F @localfirst/taco-chat test:cy",
     "test:pw": "pnpm -F @localfirst/automerge-repo-todos test:pw",

packages/auth-provider-automerge-repo/src/test/AuthProvider.test.ts

@@ -11,7 +11,7 @@ import { authenticated, authenticatedInTime } from './helpers/authenticated.js'
 import { getStorageDirectory, setup, type UserStuff } from './helpers/setup.js'
 import { synced } from './helpers/synced.js'
 
-describe.skip('auth provider for automerge-repo', () => {
+describe('auth provider for automerge-repo', () => {
   it('does not authenticate users that do not belong to any teams', async () => {
     const {
       users: { alice, bob },
@@ -184,15 +184,15 @@ describe.skip('auth provider for automerge-repo', () => {
     const bobTeam = putUserOnTeam(aliceTeam, bob)
     await bob.authProvider.addTeam(bobTeam)
 
-    // there's only one role on the team by default (ADMIN)
-    expect(bobTeam.roles()).toHaveLength(1)
+    // Teams start with ADMIN and MEMBER roles.
+    expect(bobTeam.roles()).toHaveLength(2)
 
     // Alice adds a role
     aliceTeam.addRole('MANAGERS')
 
     // Bob sees the change
     await eventPromise(bobTeam, 'updated')
-    expect(bobTeam.roles()).toHaveLength(2) // ✅
+    expect(bobTeam.roles()).toHaveLength(3) // ✅
 
     teardown()
   })
@@ -408,7 +408,7 @@ describe.skip('auth provider for automerge-repo', () => {
 // HELPERS
 
 const putUserOnTeam = (team: Auth.Team, b: UserStuff) => {
-  team.addForTesting(b.user, [], Auth.redactDevice(b.device))
+  team.addForTesting(b.user, [Auth.MEMBER], [], Auth.redactDevice(b.device))
   const serializedTeam = team.save()
   const keys = team.teamKeys()
   return Auth.loadTeam(serializedTeam, b.context, keys)

packages/auth-provider-automerge-repo/src/test/buildServerUrl.test.ts

@@ -1,7 +1,7 @@
 import { describe, it, expect } from 'vitest'
 import { buildServerUrl } from '../buildServerUrl.js'
 
-describe.skip('buildServerUrl', () => {
+describe('buildServerUrl', () => {
   it('should prepend http:// when no protocol is provided', () => {
     const { protocol, hostname } = buildServerUrl('example.com')
     expect(protocol).toBe('http:')

packages/auth-syncserver/src/test/SyncServer.test.ts

@@ -1,10 +1,10 @@
-import { createTeam, device, loadTeam } from '@localfirst/auth'
+import { createTeam, device, loadTeam, MEMBER } from '@localfirst/auth'
 import { type ShareId } from '@localfirst/auth-provider-automerge-repo'
 import { eventPromise } from '@localfirst/shared'
 import { expect, it, describe } from 'vitest'
 import { host, setup } from './helpers/setup.js'
 
-describe.skip('SyncServer', () => {
+describe('SyncServer', () => {
   it('should start a server', async () => {
     const { url } = await setup()
     const response = await fetch(`http://${url}`)
@@ -161,7 +161,7 @@ describe.skip('SyncServer', () => {
     const aliceTeam = await alice.authProvider.createTeam('team A')
 
     // Alice puts Bob on her team
-    aliceTeam.addForTesting(bob.user, [], device.redactDevice(bob.device))
+    aliceTeam.addForTesting(bob.user, [MEMBER], [], device.redactDevice(bob.device))
 
     // Alice authenticates
     await eventPromise(alice.repo.networkSubsystem, 'peer')

packages/auth/src/connection/Connection.ts

@@ -261,20 +261,25 @@ export class Connection extends EventEmitter<ConnectionEvents> {
               logger: this.logger.extend('getDeviceUser'),
             })
 
+          // A first-use device does not know its canonical userId until its invitation is accepted.
+          // Use the user recovered from the team graph before creating the device lockbox and
+          // recording the device on the team.
+          const memberDevice = { ...device, userId: user.userId }
+
           // When admitting us, our peer added our user to the team graph. We've been given the
           // serialized and encrypted graph, and the team keyring. We can now decrypt the graph and
           // reconstruct the team in order to join it.
           const team = new Team({
             source: serializedGraph,
-            context: { user, device },
+            context: { user, device: memberDevice },
             teamKeyring,
             sharedLogger: this.logger.sharedLogger,
           })
 
           // We join the team, which adds our device to the team graph.
           team.join(teamKeyring)
           this.emit('joined', { team, user, teamKeyring })
-          return { user, team }
+          return { user, device: memberDevice, team }
         }),
 
         // AUTHENTICATION
@@ -325,8 +330,13 @@ export class Connection extends EventEmitter<ConnectionEvents> {
           assert(roles)
           assert(userId)
 
-          if (!roles!.includes(MEMBER) && context.server == null && !team!.hasServer(userId!)) {
-            team!.addMemberRole(userId!, MEMBER)
+          if (
+            !roles.includes(MEMBER) &&
+            context.server == null &&
+            !team.hasServer(userId) &&
+            !team.hasServer(context.user?.userId!)
+          ) {
+            team.addMemberRole(userId, MEMBER)
           }
           this.#queueMessage('ACCEPT_IDENTITY')
         },
@@ -867,6 +877,7 @@ export class Connection extends EventEmitter<ConnectionEvents> {
       },
       error: error => {
         this.logger.error('Connection encountered an unhandled error', error)
+        console.error('error', error)
         this.#messageQueue.send(createErrorMessage(UNHANDLED, 'REMOTE'))
         this.emit('localError', { type: UNHANDLED, message: 'Unhandled error' })
         this.#fail(UNHANDLED)

packages/auth/src/connection/test/authentication.test.ts

@@ -247,8 +247,8 @@ describe('connection', () => {
 
       it('admits a first-use device before continuing authentication', async () => {
         const { bob } = setup('bob')
-        bob.team.addRole(MEMBER)
-        const { userId: _userId, ...phone } = bob.phone!
+        const phone = bob.phone!
+        const { userId: _userId } = phone
         const { seed } = bob.team.inviteDevice()
         const phoneContext: InviteeDeviceContext = {
           userName: bob.userName,

packages/auth/src/connection/test/sync.test.ts

@@ -178,7 +178,7 @@ describe('connection', () => {
         expect(charlie.team.hasRole('managers')).toBe(true)
       })
 
-      it('syncs up  three ways - changes made after connecting', async () => {
+      it('syncs up three ways - changes made after connecting', async () => {
         const { alice, bob, charlie } = setup('alice', 'bob', 'charlie')
 
         // 👩🏾<->👨🏻‍🦲<->👳🏽‍♂️ Alice, Bob, and Charlie all connect to each other

packages/auth/src/team/Team.ts

@@ -111,6 +111,11 @@ export class Team extends EventEmitter<TeamEvents> {
       const lockboxTeamKeysForMember = lockbox.create(options.teamKeys, user.keys)
       const adminKeys = createKeyset(ADMIN_SCOPE, this.seed)
       const lockboxAdminKeysForMember = lockbox.create(adminKeys, user.keys)
+      const memberKeys = createKeyset({ type: KeyType.ROLE, name: MEMBER }, this.seed)
+      const memberRoleLockboxes = [
+        lockbox.create(memberKeys, adminKeys),
+        lockbox.create(memberKeys, user.keys),
+      ]
 
       // We also store the founding user's keys in a lockbox for the user's device
       const lockboxUserKeysForDevice = lockbox.create(user.keys, this.context.device.keys)
@@ -120,7 +125,12 @@ export class Team extends EventEmitter<TeamEvents> {
         name: options.teamName,
         rootMember: redactUser(user),
         rootDevice: devices.redactDevice(device),
-        lockboxes: [lockboxTeamKeysForMember, lockboxAdminKeysForMember, lockboxUserKeysForDevice],
+        lockboxes: [
+          lockboxTeamKeysForMember,
+          lockboxAdminKeysForMember,
+          ...memberRoleLockboxes,
+          lockboxUserKeysForDevice,
+        ],
       }
 
       // Create CRDX store
@@ -253,6 +263,10 @@ export class Team extends EventEmitter<TeamEvents> {
     return select.members(this.state, userIdOrIds, options) // Many members
   }
 
+  public hasMember(userId: string): boolean {
+    return select.hasMember(this.state, userId)
+  }
+
   /**
    * Adds a member to the team, along with an (optional) device. Since this method assumes that you
    * know the member's secret keys, it only makes sense for unit tests. In real-world scenarios,
@@ -275,8 +289,8 @@ export class Team extends EventEmitter<TeamEvents> {
 
       // Post the member to the graph
       this.dispatch({
-        type: 'ADD_MEMBER',
-        payload: { member, roles: [...member.roles, ...rolesWithoutLockboxes], lockboxes },
+        type: 'ADD_MEMBER_TEST',
+        payload: { member, roles: member.roles, lockboxes },
       })
     }
 
@@ -292,6 +306,12 @@ export class Team extends EventEmitter<TeamEvents> {
 
   /** Remove a member from the team */
   public remove = (userId: string) => {
+    this.logger.debug('Removing user', userId)
+    if (!this.hasMember(userId)) {
+      this.logger.warn('Attempted to remove nonexistent member', userId)
+      return
+    }
+
     // Create new keys & lockboxes for any keys this person had access to
     const { lockboxes, updatedUserKeys } = this.rotateKeys({ type: USER, name: userId })
 
@@ -358,6 +378,8 @@ export class Team extends EventEmitter<TeamEvents> {
       }
     }
 
+    this.logger.debug('Adding role', role.roleName)
+
     // We're creating this role so we need to generate new keys
     const roleKeys = createKeyset({ type: KeyType.ROLE, name: role.roleName }, this.seed)
 
@@ -380,6 +402,7 @@ export class Team extends EventEmitter<TeamEvents> {
 
   /** Remove a role from the team */
   public removeRole = (roleName: string) => {
+    this.logger.debug('Removing role', roleName)
     this._isRoleRemovable(roleName, true)
 
     this.dispatch({
@@ -507,20 +530,19 @@ export class Team extends EventEmitter<TeamEvents> {
 
   /** Remove a member's device */
   public removeDevice = (deviceId: string) => {
+    this.logger.debug('Removing device', deviceId)
     if (!this.hasDevice(deviceId)) throw new Error(`Device ${deviceId} not found`)
 
     // Create new keys & lockboxes for any keys this device had access to
-    const { lockboxes, updatedUserKeys } = this.rotateKeys({ type: DEVICE, name: deviceId })
-
-    // update the keys on the member records
-    this.updateMemberKeysWithLockboxes(updatedUserKeys, lockboxes)
+    const { lockboxes, updatedUserKeys } = this.rotateKeys({ type: DEVICE, name: deviceId }, true)
 
     // Post the removal to the graph
     this.dispatch({
       type: 'REMOVE_DEVICE',
       payload: {
         deviceId,
         lockboxes,
+        updatedUserKeys: [...updatedUserKeys],
       },
     })
   }
@@ -743,7 +765,6 @@ export class Team extends EventEmitter<TeamEvents> {
 
     const lockboxUserKeysForDevice = lockbox.create(user.keys, device.keys)
 
-    this.logger.debug('Adding device on join')
     this.dispatch(
       {
         type: 'ADD_DEVICE',
@@ -784,6 +805,7 @@ export class Team extends EventEmitter<TeamEvents> {
    * other.)
    */
   public addServer = (server: Server) => {
+    this.logger.debug('Adding server', server.host)
     const lockboxes = this.createMemberLockboxes(castServer.toMember(server))
 
     this.dispatch({
@@ -794,7 +816,8 @@ export class Team extends EventEmitter<TeamEvents> {
 
   /** Removes a server from the team. */
   public removeServer = (host: string) => {
-    const { lockboxes } = this.rotateKeys({ type: KeyType.SERVER, name: host })
+    this.logger.debug('Removing server', host)
+    const { lockboxes } = this.rotateKeys({ type: KeyType.SERVER, name: host }, true)
     this.dispatch({
       type: 'REMOVE_SERVER',
       payload: { host, lockboxes },
@@ -950,6 +973,7 @@ export class Team extends EventEmitter<TeamEvents> {
   public changeKeys = (newKeys: KeysetWithSecrets) => {
     const { device, user } = this.context
     const { type } = newKeys
+    this.logger.debug('Changing user or device keys', type)
 
     assert(type !== DEVICE, "Can't change device keys")
     const isForUser = type === USER
@@ -987,6 +1011,7 @@ export class Team extends EventEmitter<TeamEvents> {
   }
 
   private checkForPendingKeyRotations() {
+    this.logger.debug('Checking for pending key rotations')
     // Only admins can rotate keys
     if (!this.memberIsAdmin(this.userId)) {
       return
@@ -999,10 +1024,10 @@ export class Team extends EventEmitter<TeamEvents> {
         type: USER,
         name: this.userId,
       })
-      this.dispatch({ type: 'ROTATE_KEYS', payload: { userId, lockboxes } })
-
-      // update the keys on the member records
-      this.updateMemberKeysWithLockboxes(updatedUserKeys, lockboxes)
+      this.dispatch({
+        type: 'ROTATE_KEYS',
+        payload: { userId, lockboxes, updatedUserKeys: [...updatedUserKeys] },
+      })
     }
   }
 
@@ -1025,8 +1050,8 @@ export class Team extends EventEmitter<TeamEvents> {
    * @param compromised If `compromised` is a keyset, that will become the new keyset for the
    * compromised scope. If it is just a scope, new keys will be randomly generated for that scope.
    */
-  private readonly rotateKeys = (compromised: KeyScope | KeysetWithSecrets): RotatedLockboxesWithUpdatedUserKeys => {
-    this.logger.debug('rotating keys for scope', getScope(compromised))
+  private readonly rotateKeys = (compromised: KeyScope | KeysetWithSecrets, removed = false): RotatedLockboxesWithUpdatedUserKeys => {
+    this.logger.debug('Rotating keys for scope', getScope(compromised), removed)
     const newKeyset = isKeyset(compromised)
       ? compromised // We're given a keyset - use it as the new keys
       : createKeyset(compromised) // We're just given a scope - generate new keys for it
@@ -1051,6 +1076,10 @@ export class Team extends EventEmitter<TeamEvents> {
         // Check whether we have new keys for the recipient of this lockbox
         const updatedKeyset = newKeysets.find(k => scopesMatch(k, oldLockbox.recipient))
         const updatedRecipientKeys = updatedKeyset ? redactKeys(updatedKeyset) : undefined
+        // we don't want to write new keys to the compromised scope if that scope was removed (e.g. when removing a device)
+        if (updatedRecipientKeys != null && removed && updatedRecipientKeys.type === compromised.type && updatedRecipientKeys.name === compromised.name) {
+          return undefined
+        }
         const newLockbox = lockbox.rotate({
           oldLockbox,
           newContents: newKeyset,
@@ -1059,7 +1088,7 @@ export class Team extends EventEmitter<TeamEvents> {
         })
         _addUpdatedUserKeys(updatedRecipientKeys)
         return newLockbox
-      })
+      }).filter((lockbox): lockbox is lockbox.Lockbox => lockbox != null)
     })
 
     return {
@@ -1068,6 +1097,15 @@ export class Team extends EventEmitter<TeamEvents> {
     }
   }
 
+  /**
+   * After rotation user keys need to be updated, if necessary
+   * 
+   * NOTE: some cases (e.g. removing a user) produce new keys for a given user but we don't want to update their keys since they won't propagate
+   * 
+   * @param newUserKeys Set of USER keysets that were updated during rotation
+   * @param lockboxes Lockboxes generated during rotation
+   * @param skipUserIds User IDs that we shouldn't update
+   */
   private readonly updateMemberKeysWithLockboxes = (newUserKeys: Set<Keyset>, lockboxes: lockbox.Lockbox[], skipUserIds: string[] = []): void => {
     for (const keyset of newUserKeys) {
       if (skipUserIds.includes(keyset.name)) {

packages/auth/src/team/createTeam.ts

@@ -10,5 +10,11 @@ export function createTeam(teamName: string, context: LocalContext, seed?: strin
   const defaultMetadata: TeamMetadata = {
     selfAssignableRoles: []
   }
-  return new Team({ teamName, context, teamKeys, metadata: metadata ?? defaultMetadata, sharedLogger })
+  return new Team({
+    teamName,
+    context,
+    teamKeys,
+    metadata: metadata ?? defaultMetadata,
+    sharedLogger,
+  })
 }