Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMMd35cd9589da6: [keyserver] Write MariaDB migration to remove incorrect permission from roles

Summary

Now that we prevent descendant_open_voiced from being a universal community permission for all custom roles in D9493 and fixed up the permissions associated with each user surfaced permission in D9686, we need to fix existing roles. There's two steps here:

A MariaDB migration to update roles and memberships
A redux-persist migration that updates the clients thread store

This diff will handle just step 1. Here, we map over all of the permissions that need to be removed (listed in permissionsToRemoveInMigration) and create a dynamic SET clause. I used this guide to write a JSON_REMOVE query that takes multiple paths. The JSON_REMOVE clause looks a little like JSON_REMOVE(permissions, '$.descendant_open_voiced', '$.descendant_manage_pins', '$.descendant_edit_messages', ...)

This will not be landed until the persist migration is up for review + accepted as well so they can land at the same time

Depends on D9686

One part of ENG-5254

Test Plan

Ran the validate-role-permissions.js script to confirm the success of the migration.

Before running the migration:

yarn run v1.22.19
$ . bash/source-nvm.sh && NODE_ENV=development node --loader=./loader.mjs dist/scripts/validate-role-permissions.js
(node:42854) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
====================================
Validating: Role Name (Members) | Role ID (92379) | Thread Type (8) | Thread ID (92378)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Admins) | Role ID (92380) | Thread Type (8) | Thread ID (92378)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Members) | Role ID (92406) | Thread Type (8) | Thread ID (92405)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {
  "descendant_open_voiced": true,
  "join_thread": true,
  "descendant_react_to_message": true,
  "descendant_edit_message": true,
  "descendant_add_members": true,
  "descendant_edit_entries": true,
  "descendant_edit_thread": true,
  "descendant_edit_thread_description": true,
  "descendant_edit_thread_color": true,
  "descendant_toplevel_create_subthreads": true,
  "descendant_edit_thread_avatar": true,
  "descendant_toplevel_create_sidebars": true
}

Potential permission discrepancies for role Members that could be linked back to user surfaced permissions (i.e. not an actual discrepancy, but rather a user edited a role): 

userSurfacedExistingPermissionsToExpectedPermissions = [
  "edit_calendar",
  "create_and_edit_channels",
  "add_members",
  "react_to_messages",
  "edit_messages"
]
====================================
Validating: Role Name (Admins) | Role ID (92407) | Thread Type (8) | Thread ID (92405)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Members) | Role ID (92469) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {
  "descendant_open_voiced": true,
  "join_thread": true,
  "descendant_react_to_message": true,
  "descendant_edit_message": true,
  "descendant_add_members": true
}

Potential permission discrepancies for role Members that could be linked back to user surfaced permissions (i.e. not an actual discrepancy, but rather a user edited a role): 

userSurfacedExistingPermissionsToExpectedPermissions = [
  "add_members",
  "react_to_messages",
  "edit_messages"
]
====================================
Validating: Role Name (Admins) | Role ID (92470) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Helpers) | Role ID (92495) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {
  "manage_pins": true,
  "descendant_manage_pins": true,
  "react_to_message": true,
  "descendant_react_to_message": true,
  "edit_message": true,
  "descendant_edit_message": true,
  "manage_invite_links": true,
  "descendant_manage_invite_links": true,
  "change_role": true,
  "descendant_change_role": true,
  "edit_permissions": true,
  "descendant_edit_permissions": true
}

Potential permission discrepancies for role Helpers that could be linked back to user surfaced permissions (i.e. not an actual discrepancy, but rather a user edited a role): 

userSurfacedExistingPermissionsToExpectedPermissions = [
  "change_roles",
  "edit_visibility",
  "manage_pins",
  "react_to_messages",
  "edit_messages",
  "manage_invite_links"
]
====================================
✨  Done in 0.32s.

After running just migration #51:

yarn run v1.22.19
$ . bash/source-nvm.sh && NODE_ENV=development node --loader=./loader.mjs dist/scripts/validate-role-permissions.js
(node:46744) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
====================================
Validating: Role Name (Members) | Role ID (92379) | Thread Type (8) | Thread ID (92378)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Admins) | Role ID (92380) | Thread Type (8) | Thread ID (92378)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Members) | Role ID (92406) | Thread Type (8) | Thread ID (92405)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Admins) | Role ID (92407) | Thread Type (8) | Thread ID (92405)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Members) | Role ID (92469) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Admins) | Role ID (92470) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {}

====================================
Validating: Role Name (Helpers) | Role ID (92495) | Thread Type (9) | Thread ID (92468)

deepDiff(baseExpectedPermissionBlob, existingRolePermissions) = {}

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {
  "manage_pins": true,
  "react_to_message": true,
  "edit_message": true,
  "manage_invite_links": true,
  "change_role": true,
  "edit_permissions": true
}

Potential permission discrepancies for role Helpers that could be linked back to user surfaced permissions (i.e. not an actual discrepancy, but rather a user edited a role): 

userSurfacedExistingPermissionsToExpectedPermissions = [
  "change_roles",
  "edit_visibility",
  "manage_pins",
  "react_to_messages",
  "edit_messages",
  "manage_invite_links"
]
====================================
✨  Done in 0.32s.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Oct 25 2023, 8:29 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 25 2023, 8:29 AM

rohan edited the test plan for this revision. (Show Details)Oct 25 2023, 8:32 AM

rohan attached a referenced file: F837290: Screenshot 2023-10-25 at 11.16.19 AM.png. (Show Details)

rohan attached a referenced file: F837289: Screenshot 2023-10-25 at 11.12.52 AM.png. (Show Details)

rohan edited the test plan for this revision. (Show Details)Oct 25 2023, 8:37 AM

rohan added inline comments.Oct 25 2023, 8:40 AM

keyserver/src/database/migration-config.js
587–594 ↗	(On Diff #32404)	The syntax seems to be mixed in the file for whether to start the `SQL` on a new line or not. I'm happy to do it either way

rohan edited the summary of this revision. (Show Details)Oct 25 2023, 8:40 AM

Harbormaster completed remote builds in B23492: Diff 32404.Oct 25 2023, 8:48 AM

rohan requested review of this revision.Oct 25 2023, 8:48 AM

rohan added a child revision: D9608: [native] Redux persist migration to remove incorrect permissions from threads.Oct 26 2023, 9:01 AM

Thanks for including documentation in Test Plan to show that the migration works as expected. Adding @ashoat as blocking reviewer to take another look since DB migrations can be "risky."

keyserver/src/database/migration-config.js
587–594 ↗	(On Diff #32404)	Whether or not to end query in semi-colon also seems mixed. Personally prefer queries to end with semi-colon, but I think the "team standard" is to exclude them unless there are multiple statements.

This revision is now accepted and ready to land.Oct 26 2023, 10:10 AM

Adding @ashoat as blocking per @atul's request

This revision now requires review to proceed.Oct 26 2023, 10:19 AM

In D9599#281451, @rohan wrote:

Adding @ashoat as blocking per @atul's request

Oops sorry, missed "Change Reviewer" step.. thanks for fixing that.

I ran this query to make sure admins don't have descendant_open_voiced and it looks safe:

MariaDB [comm_september2023]> SELECT * FROM roles WHERE JSON_EXTRACT(permissions, '$.descendant_open_voiced') = 'true' AND name = 'Admins' LIMIT 10;
Empty set (0.011 sec)

However, I also ran this query and it didn't return as many results as I expected:

MariaDB [comm_september2023]> SELECT COUNT(*) FROM roles WHERE JSON_EXTRACT(permissions, '$.descendant_open_voiced') = 'true';
+----------+
| COUNT(*) |
+----------+
|        5 |
+----------+

Looking more closely, the only roles where this permission appears are communities where custom roles were created, AND the Members role in the Comm org.

Given it appears in universalCommunityPermissions on master currently, I would've expected to see this appear in more places. But looking at the code, universalCommunityPermissions only appears to be used in modifyRole.

Two questions:

Should we do anything to make sure that in-database role permissions match what the code expects? Otherwise, it feels risky... we wait until modifyRole is called, and then some unexpected permission can be added / removed.
- We could potentially create a script that runs and compare the database contents to what the code expects.
Shouldn't we be checking universalCommunityPermissions when a role is created, and not just when it's modified?
- I'm concerned that your process of building this was focused only on the "new surface" you were introducing, and you skipped trying to enforce any consistency with the existing surfaces. We should have consistency between modification / creation.

keyserver/src/database/migration-config.js
587–594 ↗	(On Diff #32404)	Does the `WHERE` clause here improve perf? It seems like it'll be a no-op for the rows that don't match the `WHERE`... wondering if MySQL is smart enough to "skip" the no-op, in which case it might have better perf to remove the `WHERE` Yeah we generally skip semicolons unless we're using `multipleStatements: true`

This revision now requires changes to proceed.Oct 26 2023, 12:57 PM

Please don't land this without creating follow-up tasks

Two tasks ENG-5621 and ENG-5622 are tracking the feedback provided to unblock this diff once I update the query to remove the semicolon

keyserver/src/database/migration-config.js
587–594 ↗	(On Diff #32404)	I don't think MySQL is skipping the rows that don't match. I ran the following two queries on two identical setups of `roles`: 103 total rows in the database two are `Admins` and `Members` the other 101 roles are custom created roles with a script Only the custom roles will have `descendant_open_voiced` in this setup since I'm not touching `Members` By excluding the `WHERE` clause, 103 rows are affected with the query UPDATE roles SET permissions = JSON_REMOVE(permissions, '$.descendant_open_voiced'); By including the `WHERE` clause, 101 rows are affected with the query UPDATE roles SET permissions = JSON_REMOVE(permissions, '$.descendant_open_voiced') WHERE JSON_EXTRACT(permissions, '$.descendant_open_voiced') = 'true' I think ultimately a few rows here or there aren't going to be a big deal, I just felt it safer to include the `WHERE`. If you disagree, I will remove the `WHERE` in the same update as I fix the semicolon here

Got it, let's keep the WHERE. Only feedback to apply here is stripping the semicolon.

That said, I think it would be best to wait on landing this diff. Each updateRolesAndPermissionsForAllThreads takes my keyserver down for 15-30min while it runs, and I suspect that we'll need to introduce another migration for ENG-5183. I'd rather introduce a single updateRolesAndPermissionsForAllThreads at the end of all of this work, rather than introducing two of them (one now and one later).

This revision is now accepted and ready to land.Nov 1 2023, 9:55 AM

In D9599#282817, @ashoat wrote:

That said, I think it would be best to wait on landing this diff. Each updateRolesAndPermissionsForAllThreads takes my keyserver down for 15-30min while it runs, and I suspect that we'll need to introduce another migration for ENG-5183. I'd rather introduce a single updateRolesAndPermissionsForAllThreads at the end of all of this work, rather than introducing two of them (one now and one later).

Got it, I'll keep this stack open then in the meantime

Remove semicolon from query

Harbormaster completed remote builds in B23646: Diff 32597.Nov 1 2023, 11:53 AM

rohan mentioned this in D9675: [keyserver] Write a script to compare database role permissions match expectations.Nov 2 2023, 9:04 AM

rohan mentioned this in rCOMMb49929e80a2b: [keyserver] Write a script to compare database role permissions match….Nov 2 2023, 6:56 PM

rohan planned changes to this revision.Nov 3 2023, 10:36 AM

rohan removed a parent revision: D9493: [lib] Fix universal community permissions to prevent input bar and join chat button from appearing at the same time.Nov 3 2023, 10:58 AM

rohan added a parent revision: D9686: [lib] Prevent editing role permissions from adding unexpected permissions.

rohan edited the summary of this revision. (Show Details)Nov 6 2023, 7:18 AM

rohan edited the test plan for this revision. (Show Details)

rohan added a parent revision: D9493: [lib] Fix universal community permissions to prevent input bar and join chat button from appearing at the same time.

rohan retitled this revision from [keyserver] Write MariaDB migration to remove 'descendant_open_voiced' permission from roles to [keyserver] Write MariaDB migration to remove incorrect permission from roles.

Update MariaDB migration (and description/title of this diff) to remove all incorrect permissions from roles

This revision is now accepted and ready to land.Nov 6 2023, 7:20 AM

rohan requested review of this revision.Nov 6 2023, 7:21 AM

Reduce line length

Harbormaster completed remote builds in B23795: Diff 32783.Nov 6 2023, 8:02 AM

Harbormaster completed remote builds in B23797: Diff 32786.Nov 6 2023, 8:07 AM

ashoat added inline comments.Nov 6 2023, 8:18 AM

keyserver/src/database/migration-config.js
650 ↗	(On Diff #32786)	Confused about this conditional... it appears that if it's false, we'll be attempting an `UPDATE` without a `SET`. Is that even valid SQL? Should we just skip the migration in this case, or alternately should we add an `invariant` to make sure that `permissionsToRemoveInMigration` is non-empty?
656–657 ↗	(On Diff #32786)	Are these the only types where we had given `descendant_open_voiced`? (Ignoring the descendants of these, which will be handled below by migration 52)

rohan added inline comments.Nov 6 2023, 9:57 AM

keyserver/src/database/migration-config.js
650 ↗	(On Diff #32786)	Ah yeah you're right... the `invariant` in a migration is kind of scary so I'll opt to just skip the migration.
656–657 ↗	(On Diff #32786)	Logically, yes because the roles were community-level. Just to make sure though, I ran the following query and it appears that this is the case SELECT * FROM threads WHERE id IN (SELECT thread FROM roles WHERE JSON_EXTRACT(permissions, '$.descendant_open_voiced') IS NOT NULL);

Address feedback

rohan added inline comments.Nov 6 2023, 9:59 AM

keyserver/src/database/migration-config.js
649–660 ↗	(On Diff #32821)	Separating these out using `append` since it makes a lot cleaner to write / read, and also because it helps avoid really small syntactical errors of inlining the `JSON_REMOVE` set statement

rohan removed a parent revision: D9493: [lib] Fix universal community permissions to prevent input bar and join chat button from appearing at the same time.Nov 6 2023, 10:04 AM

Harbormaster completed remote builds in B23808: Diff 32821.Nov 6 2023, 10:41 AM

Make sure you use the SQL template

keyserver/src/database/migration-config.js
645 ↗	(On Diff #32821)	You forgot the `SQL` here. When you skip that you leave the code vulnerable to SQL injection. Please be careful to notice this in the future

This revision is now accepted and ready to land.Nov 6 2023, 11:37 AM

Use SQL. Made a small change to the setClause construction to avoid json syntax errors, and repeated my test plan to make sure it still worked as expected

keyserver/src/database/migration-config.js
645 ↗	(On Diff #32821)	Got it - will make sure to double check my use of `SQL` next time

rohan removed a parent revision: D9493: [lib] Fix universal community permissions to prevent input bar and join chat button from appearing at the same time.Nov 6 2023, 12:58 PM

rohan edited the summary of this revision. (Show Details)

Harbormaster failed remote builds in B23818: Diff 32832!Nov 6 2023, 1:29 PM

Harbormaster completed remote builds in B23818: Diff 32832.Nov 7 2023, 3:16 AM

Remove migration #52 (updateRolesAndPermissionsForAllThreads) since this stack isn't getting landed til ENG-5183 is ready to land, and I'll need a updateRolesAndPermissionsForAllThreads migration at the end of that anyways

Harbormaster failed remote builds in B23972: Diff 33024!Nov 9 2023, 12:29 PM

Harbormaster completed remote builds in B23972: Diff 33024.Nov 9 2023, 12:45 PM

rohan mentioned this in D9945: [keyserver] Add updateRolesAndPermissionsForAllThreads migration.Nov 21 2023, 9:21 AM

Rebase

Harbormaster completed remote builds in B24334: Diff 33491.Nov 22 2023, 7:36 AM

Closed by commit rCOMMd35cd9589da6: [keyserver] Write MariaDB migration to remove incorrect permission from roles (authored by rohan). · Explain WhyNov 22 2023, 8:48 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMMd35cd9589da6: [keyserver] Write MariaDB migration to remove incorrect permission from roles.

rohan mentioned this in rCOMMddaed8961488: [keyserver] Add updateRolesAndPermissionsForAllThreads migration.