Page MenuHomePhabricator

[keyserver] Recalculate permissions and reintroduce ADD_MEMBERS permission
ClosedPublic

Authored by ashoat on Aug 19 2024, 9:44 AM.
Tags
None
Referenced Files
F2767178: D13114.diff
Thu, Sep 19, 4:15 PM
Unknown Object (File)
Sat, Sep 14, 12:02 PM
Unknown Object (File)
Mon, Sep 9, 7:23 AM
Unknown Object (File)
Tue, Sep 3, 6:15 PM
Unknown Object (File)
Sat, Aug 31, 6:33 AM
Unknown Object (File)
Fri, Aug 30, 3:48 PM
Unknown Object (File)
Fri, Aug 30, 3:33 PM
Unknown Object (File)
Fri, Aug 30, 2:24 PM
Subscribers
None

Details

Summary

This diff does two things:

  1. Resolves ENG-9055 by bringing back ADD_MEMBERS permissions. Unfortunately we have no way to determine which communities had this permission before the errant migration, so I'm just assuming they all had it (as it's a default) and bringing it back for all communities.
  2. Recalculates all permissions so that the changes in the last two diffs in the stack are applied.

Depends on D13113

Test Plan
  1. I set up a repro of ENG-9055 in my local environment
  2. I confirmed that after this migration ran, ADD_MEMBERS permission was present

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/permissions
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/database/migration-config.js
881–895

This query was inspired by a similar one in updateRoles here

881–895

I should mention I tested its performance on the production MariaDB, as I was worried about removing the WHERE thread clause:

mysql> UPDATE roles SET permissions = CASE id WHEN '2' THEN '' ELSE permissions END;
Query OK, 0 rows affected (0.01 sec)
Rows matched: 8709  Changed: 0  Warnings: 0
tomek added inline comments.
keyserver/src/database/migration-config.js
856

Why do we skip the admin role? Shouldn't we also have ADD_MEMBERS for these?

881–895

This query might be quite long. Are we sure we don't need to split it into batches?

This revision is now accepted and ready to land.Aug 20 2024, 1:37 AM
keyserver/src/database/migration-config.js
856

Admin roles were not affected by the errant migration, as they're special-cased here.

Additionally, admin role permissions are not determined by the user-surfaced permission system, and the role editing UI does not allow editing admin permissions. Instead, admin permissions are determined directly by getRolePermissionBlobs. If we were to include them here, it would require adding some special cases below.

881–895

That's a fair concern, so I did some testing to make sure it was okay.

I constructed a fake query with the same amount of entries as follows:

let query = 'UPDATE roles SET permissions = CASE id ';
for (let i = 0; i < 8709; i++) {
  query += `WHEN '-${i}' THEN '' `;
}
query += 'ELSE permissions END';

I then ran this query on my production keyserver:

Query OK, 0 rows affected (3.24 sec)
Rows matched: 8714  Changed: 0  Warnings: 0

Technically this is just a little shorter than the real query, since the IDs are shorter. But I think it should be okay.