feat: validate role records

[EvanHahn]

Depends on #968.

When fetching a role, we now validate that the role is valid, returning a blocked role if it's not.

See #188 for more details.

[awana-lockfile-bot]

package-lock.json changes

Click to toggle table visibility

Name	Status	Previous	Current
p-reduce		-	3.0.0

[EvanHahn]

I made a few changes based on discussions with @gmaclennan. I also did some quick profiling.

I wrote a test that creates 20 managers and then invites them one by one. Manager 1 invites Manager 2, Manager 2 invites Manager 3, and so on.

Expand this to see the test code.

// This code is not perfect, but is good enough for this test.
test('perf test', async (t) => {
  const managers = await createManagers(20, t)
  const [creator] = managers

  const projectId = await creator.createProject({ name: 'Mapeo' })

  const disconnect = connectPeers(managers)
  t.after(disconnect)

  console.time('invite chain')
  /** @type {undefined | MapeoManager} */
  let previousManager
  for (const manager of managers) {
    if (previousManager) {
      console.log(previousManager.deviceId, 'inviting', manager.deviceId)
      await invite({
        projectId,
        invitor: previousManager,
        invitees: [manager],
        roleId: COORDINATOR_ROLE_ID,
      })
    }
    previousManager = manager
  }
  console.timeEnd('invite chain')

  const firstManager = managers[0]
  const firstProject = await firstManager.getProject(projectId)

  const lastManager = managers[19]

  console.time('looking up role')
  const lastManagerRole = await firstProject.$member.getById(
    lastManager.deviceId
  )
  assert.equal(lastManagerRole.role.roleId, COORDINATOR_ROLE_ID)
  console.timeEnd('looking up role')
})

Before this change, on my machine:

• doing all the invites: 41.274s
• looking up the role: 0.174ms

After this change:

• doing all the invites: 49.885s
• looking up the role: 6.961ms

Looking up the role is significantly slower, but I still think it's in an acceptable range.

[gmaclennan]

Great work on this, the logic is tricky but clear enough to follow in review.

Can you add additional test(s) for a more complex scenario, e.g. a role chain of length > 2. I think it's ok to add a TODO for testing forked membership records. For forked membership records we also need to change the getWinner for membership records (to match the logic we use for looking up links - a doc only has multiple links if it was forked and then was subsequently merged.).

I think there are a few other cases that you are handling in the code that are not in the tests, it might be helpful to run a coverage test and see what code paths within the new code are being tested right now.

I am a bit concerned about the slow-down - it's a 40x slow down, and it could be more on phones depending on whether it is CPU limited or disk speed limited. If we start checking roles for reads of all data types, this could introduce a significant overhead. I think it's ok that we do not address this right now, and we think of a solution down the line when performance becomes an issue. Before we do that, we should spend some time adding perf metrics that get reported to Sentry.

[gmaclennan]

Should this maybe be nullIfNotFound, and we should just allow it to throw if there is a different error?

[gmaclennan]

Maybe be more explicit and return false here, since that is what will happen anyway.

[gmaclennan]

I think it's slightly risky relying on strict equality here. We may make a change in the future that means that this is not strictly equal, and not realise the consequences.

I think we should match based on parentMembershipRecord.roleId === CREATOR_ROLE_ID.

This means you can't write a switch statement, which is my hidden agenda for this comment... ha.

I do think having 3 if statements will make this a bit more readable, with comments for each statement just explaining what that condition means.

[gmaclennan]

See comment below about relying on strict equality here.

[gmaclennan]

This should have a max-depth check, to avoid a bug/hack that might create a circular reference so that this never ends.

[gmaclennan]

This should have a max-depth check, to avoid a bug/hack that might create a circular reference so that this never ends.

[EvanHahn]

I spent a little more time thinking about this and I think I need to change the approach.

Currently, if we encounter an invalid membership record, we treat that member as blocked. I think that's not bad because a member could write a bogus membership forever block a user from a project.

For example, imagine the following:

1. Creator invites A and B, both as regular members.
2. A claims that B is a coordinator. This is bogus because it lacks the permission.

Currently, this PR will treat B as blocked. Instead, I think we should ignore A's claim; B's role should still be "member".

In other words, I think we should ignore invalid membership records.

I'll try to implement that next week.