Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate Role records #188

Open
1 task
gmaclennan opened this issue Aug 18, 2023 · 0 comments · May be fixed by #967
Open
1 task

Validate Role records #188

gmaclennan opened this issue Aug 18, 2023 · 0 comments · May be fixed by #967
Labels
post-mvp de-scoped to after MVP

Comments

@gmaclennan
Copy link
Member

gmaclennan commented Aug 18, 2023

Description

Validating a role record requires tracing a "role chain" back to the project creator auth core (the core whose key is the project key), and checking in each case that the role defined has the capability to assign the role given.

Because roles can change over time (e.g. a device might start as a "member", then be changed to "coordinator", then be changed to "removed"), each role record has a fromIndex property, that defines the index from which the role applies in the auth core of the person to whom the role applies.

An example:

  1. project creator invites device A, and writes their role to be 'coordinator'
  2. device A invites device B, and writes device B's role to be 'member'.

Validating device B's role requires the following steps:

  1. The role document for B will be written in device A's auth core. You can get this information from the versionId of the role document.
  2. Look up the role of device A to check they have authority to assign a 'member' role:
    1. The role document of device A is written by the project creator, so we trust the "role chain".
    2. The role of A is 'coordinator', which has the capability to assign 'member' roles, so this role chain is valid.

What is a "Role chain"

A "Role chain" is the chain of role assignments that can be traced back to the project creator. E.g. project creator assigns device A a role, then device A assigns a role to device B, and device B assigns a role to device C. This "chain" of role assignments from creator -> A -> B -> C we call a "role chain" (suggestions for a better name are welcome!). Using the core key of the auth core as the docId for each role document, combined with the key of the auth core where the role document is written, can be used to validate this role chain.

Validation flowchart

This flowchart summarises all the steps to take to validate any role:

flowchart TD
    Invalid([Role chain <br/>is INVALID])
    Valid([Role chain <br/>is VALID])
    Start([Start: want to validate <br/>role of coreKey]) --> A
    A[Get Role from coreKey] --> B{Is role written in <br/>project creator core?}
    B ---> |Yes| Valid
    B --> |No| C[Get GrantorRole from <br/>coreKey of role record]
    C --> D{Is index of role <br/>record >= indexFrom <br/>in GrantorRole?}
    D --> |No| E{Does grantorRole link <br/>to previous versions?}
    E ---> |No| Invalid
    E --> |Yes| F[Get previous version <br/>of grantorRole]
    F --> D
    D --> |Yes| G{Does grantorRole <br/>allow assigning role?}
    G ---> |No| Invalid
    G ---> |Yes| H[Now validate <br/>grantorRole as role]
    H --> B
Loading

Validating fromIndex

There is currently no way to validate that fromIndex was actually the length of the auth core of the device assigned the role at the time the role document was written. This could be used to "back-date" a role assignment.

However this is also a feature, for example "device X" is in a coordinator role, and turns "bad". Device X invites other bad actors to the project. Another device that has a role with the capability to assign the "blocked" role (the prevents sync) can assign that role with a fromIndex of 0. That would mean that any device with a role that came from X would be considered an invalid role, and be blocked. If any of these devices were considered OK (e.g. not bad actors) then another device could assign them a role, again with a "back-dated" fromIndex, which would give them a valid role chain back to when they first joined the project.

The one way that fromIndex could be exploited is when two bad actors manage to coordinate - e.g. one device with more capabilities could backdate capabilities to another device. However a bad actor with a role with a lot of capabilities like this could do lots of other bad things without backdating (inviting other bad actors), so there needs to be some implicit trust in members of a project. If a bad actor is discovered, then they can be removed (along with anyone they have assigned a role to) as described above.

If we want some extra security about backdating, we could, in the future, add a method to analyze the auth cores, and see where it has happened. To do this will will need to add an extra property to every record written to the auth store that includes a "vector clock", e.g. a map of auth core keys to their current length. This would give a way to check whether fromIndex was written with a value other than the length of the core at the time. It remains to be seen however if this is a valuable feature and whether manipulating this is actually a threat.

Tasks

  • Add the validation steps outlined in the flowchart above to capabilities.getRole(deviceId).
@gmaclennan gmaclennan added the post-mvp de-scoped to after MVP label Aug 18, 2023
@EvanHahn EvanHahn self-assigned this Nov 12, 2024
EvanHahn added a commit that referenced this issue Nov 12, 2024
This adds a new option to `DataType.prototype.getByDocId()`,
`mustBeFound`. If set to `false` and the document doesn't exist, it will
resolve with `null` instead of throwing.

If the option is missing or set to `true`, the current behavior is
maintained (throwing if the document is missing).

I think this is a useful feature on its own but will also hopefully make
at least one [upcoming change][0] a little easier.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 12, 2024
This adds a new option to `DataType.prototype.getByDocId()`,
`mustBeFound`. If set to `false` and the document doesn't exist, it will
resolve with `null` instead of throwing.

If the option is missing or set to `true`, the current behavior is
maintained (throwing if the document is missing).

I think this is a useful feature on its own but will also hopefully make
at least one [upcoming change][0] a little easier.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 12, 2024
(This diff looks large, but it's just one line of source code and a
bunch of tests.)

We have some code like this:

```
if (isAssigningProjectCreatorRole && !this.#isProjectCreator()) {
```

The intent: only allow the project creator to change their own role.

However, `this.#isProjectCreator` returned a `Promise`, which meant that
the second part of the condition *always* evaluated to `false`, which
meant that the whole condition always evaluated to false, which meant
that non-creators could change the creator's role.

This fixes that by making `#isProjectCreator` return a `boolean`, not
`Promise<boolean>`.

Found this while working on [#188].

[#188]: #188
EvanHahn added a commit that referenced this issue Nov 12, 2024
This adds a new option to `DataType.prototype.getByDocId()`,
`mustBeFound`. If set to `false` and the document doesn't exist, it will
resolve with `null` instead of throwing.

If the option is missing or set to `true`, the current behavior is
maintained (throwing if the document is missing).

I think this is a useful feature on its own but will also hopefully make
at least one [upcoming change][0] a little easier.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 14, 2024
This change should have no impact on functionality, and is a minor
change.

In [an upcoming change][0], I plan to make several references to the
blocked role. This defines the `BLOCKED_ROLE` constant for this purpose.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 14, 2024
This change should have no impact on functionality, and is a minor
change.

In [an upcoming change][0], I plan to make several references to the
blocked role. This defines the `BLOCKED_ROLE` constant for this purpose.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 18, 2024
(This diff looks large, but it's just one line of source code and a
bunch of tests.)

We have some code like this:

```
if (isAssigningProjectCreatorRole && !this.#isProjectCreator()) {
```

The intent: only allow the project creator to change their own role.

However, `this.#isProjectCreator` returned a `Promise`, which meant that
the second part of the condition *always* evaluated to `false`, which
meant that the whole condition always evaluated to false, which meant
that non-creators could change the creator's role.

This fixes that by making `#isProjectCreator` return a `boolean`, not
`Promise<boolean>`.

Found this while working on [#188].

[#188]: #188
EvanHahn added a commit that referenced this issue Nov 18, 2024
This makes the following kind of change in a bunch of places:

```diff
-throw new Error('Not found')
+throw new NotFoundError()
```

I think this is a useful improvement on its own, but the changes to
DataStore will also make [an upcoming change easier][0].

[0]: #188
EvanHahn added a commit that referenced this issue Nov 18, 2024
This adds a new option to `DataType.prototype.getByDocId()`,
`mustBeFound`. If set to `false` and the document doesn't exist, it will
resolve with `null` instead of throwing.

If the option is missing or set to `true`, the current behavior is
maintained (throwing if the document is missing).

I think this is a useful feature on its own but will also hopefully make
at least one [upcoming change][0] a little easier.

[0]: #188
EvanHahn added a commit that referenced this issue Nov 18, 2024
This makes the following kind of change in a bunch of places:

```diff
-throw new Error('Not found')
+throw new NotFoundError()
```

I think this is a useful improvement on its own, but the changes to
DataStore will also make [an upcoming change easier][0].

[0]: #188
EvanHahn added a commit that referenced this issue Nov 18, 2024
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.

[0]: #188
@EvanHahn EvanHahn linked a pull request Nov 19, 2024 that will close this issue
EvanHahn added a commit that referenced this issue Nov 19, 2024
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.

[0]: #188
@EvanHahn EvanHahn removed their assignment Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-mvp de-scoped to after MVP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants