From 2ae1836b9ad75464a4f85d3eabb05a66a741ff7c Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 16:58:51 +0100
Subject: [PATCH] PR comments.

---
 packages/backend-core/src/security/roles.ts      |  8 +++-----
 packages/server/src/api/controllers/role.ts      |  6 +++---
 .../src/tests/utilities/TestConfiguration.ts     | 16 ++++------------
 .../shared-core/src/helpers/tests/roles.spec.ts  |  5 +++++
 4 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index b3c18202d2..366e74bec5 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -76,11 +76,9 @@ export class Role implements RoleDoc {
     if (inherits && typeof inherits === "string") {
       inherits = prefixRoleIDNoBuiltin(inherits)
     } else if (inherits && Array.isArray(inherits)) {
-      inherits = inherits.map(inherit => prefixRoleIDNoBuiltin(inherit))
-    }
-    if (inherits) {
-      this.inherits = inherits
+      inherits = inherits.map(prefixRoleIDNoBuiltin)
     }
+    this.inherits = inherits
     return this
   }
 }
@@ -264,7 +262,7 @@ export async function roleToNumber(id: string) {
             return findNumber(foundRole) + 1
           }
         })
-        .filter(number => !!number)
+        .filter(number => number)
         .sort()
         .pop()
       if (highestBuiltin != undefined) {
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index fef68a650c..89125f566e 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -78,10 +78,10 @@ export async function find(ctx: UserCtx<void, FindRoleResponse>) {
 
 export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
   const db = context.getAppDB()
-  let { _id, name, inherits, permissionId, version, uiMetadata } =
+  let { _id, _rev, name, inherits, permissionId, version, uiMetadata } =
     ctx.request.body
   let isCreate = false
-  if (!ctx.request.body._rev && !version) {
+  if (!_rev && !version) {
     version = roles.RoleIDVersion.NAME
   }
   const isNewVersion = version === roles.RoleIDVersion.NAME
@@ -132,7 +132,7 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     ctx.throw(400, "Role inheritance contains a loop, this is not supported")
   }
 
-  const foundRev = ctx.request.body._rev || dbRole?._rev
+  const foundRev = _rev || dbRole?._rev
   if (foundRev) {
     role._rev = foundRev
   }
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index fc5a4a7f9c..ca0a6e420a 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -432,7 +432,7 @@ export default class TestConfiguration {
   async loginAsRole(roleId: string, cb: () => Promise<unknown>) {
     const roleUser = await this.createUser({
       roles: {
-        [this.prodAppId!]: roleId,
+        [this.getProdAppId()]: roleId,
       },
       builder: { global: false },
       admin: { global: false },
@@ -443,17 +443,9 @@ export default class TestConfiguration {
       builder: false,
       prodApp: true,
     })
-    const temp = this.user
-    this.user = roleUser
-    await cb()
-    if (temp) {
-      this.user = temp
-      await this.login({
-        userId: temp._id!,
-        builder: true,
-        prodApp: false,
-      })
-    }
+    await this.withUser(roleUser, async () => {
+      await cb()
+    })
   }
 
   defaultHeaders(extras = {}, prodApp = false) {
diff --git a/packages/shared-core/src/helpers/tests/roles.spec.ts b/packages/shared-core/src/helpers/tests/roles.spec.ts
index 7b67e4e9e9..051a196a5b 100644
--- a/packages/shared-core/src/helpers/tests/roles.spec.ts
+++ b/packages/shared-core/src/helpers/tests/roles.spec.ts
@@ -64,5 +64,10 @@ describe("role utilities", () => {
       role1.inherits = "role_2"
       check(true)
     })
+
+    it("self reference contains loop", () => {
+      role("role1", "role1")
+      check(true)
+    })
   })
 })