Refactor multi-namespace self access checks to avoid flapping of UI b…

…uttons

Signed-off-by: Kevin Cormier <[email protected]>

frontend/src/components/LoadData.tsx

@@ -2,7 +2,7 @@
 import { Fragment, ReactNode, useContext, useEffect, useMemo, useState } from 'react'
 import { PluginDataContext } from '../lib/PluginDataContext'
 // eslint-disable-next-line @typescript-eslint/no-restricted-imports
-import { SetterOrUpdater, useSetRecoilState } from 'recoil'
+import { SetterOrUpdater, useRecoilValue, useSetRecoilState } from 'recoil'
 import { tokenExpired } from '../logout'
 import {
   AgentClusterInstallApiVersion,
@@ -175,7 +175,6 @@ import {
   WatchEvent,
 } from '../atoms'
 import { useQuery } from '../lib/useQuery'
-import { useRecoilValue } from '../shared-recoil'
 import get from 'lodash/get'
 
 export function LoadData(props: { children?: ReactNode }) {

frontend/src/lib/rbac-util.test.ts

@@ -2,7 +2,7 @@
 
 import { Namespace, NamespaceDefinition } from '../resources'
 import { nockIgnoreApiPaths, nockRBAC } from './nock-util'
-import { getAuthorizedNamespaces, isAnyNamespaceAuthorized } from './rbac-util'
+import { areAllNamespacesUnauthorized, getAuthorizedNamespaces, isAnyNamespaceAuthorized } from './rbac-util'
 import { waitForNocks } from './test-util'
 
 const adminAccess = { name: '*', namespace: '*', resource: '*', verb: '*' }
@@ -49,43 +49,81 @@ describe('isAnyNamespaceAuthorized', () => {
       nockRBAC({ ...createDeployment, namespace: 'test-namespace-2' }, true),
     ]
     expect(
-      await isAnyNamespaceAuthorized(
-        [createDeployment],
-        [
-          {
-            ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
-            metadata: { name: 'test-namespace-1' },
-          },
-          {
-            ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
-            metadata: { name: 'test-namespace-2' },
-          },
-        ]
-      )
+      await isAnyNamespaceAuthorized(Promise.resolve(createDeployment), [
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-1' },
+        },
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-2' },
+        },
+      ]).promise
     ).toEqual(true)
     await waitForNocks(nocks)
   })
   it('returns false for an empty namespace list', async () => {
-    expect(await isAnyNamespaceAuthorized([createDeployment], [])).toEqual(false)
+    expect(await isAnyNamespaceAuthorized(Promise.resolve(createDeployment), []).promise).toEqual(false)
   })
   it('returns true without checking namespaces for an admin user', async () => {
     nockIgnoreApiPaths()
     const nocks = [nockRBAC(adminAccess, true)]
     expect(
-      await isAnyNamespaceAuthorized(
-        [createDeployment],
-        [
-          {
-            ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
-            metadata: { name: 'test-namespace-1' },
-          },
-          {
-            ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
-            metadata: { name: 'test-namespace-2' },
-          },
-        ]
-      )
+      await isAnyNamespaceAuthorized(Promise.resolve(createDeployment), [
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-1' },
+        },
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-2' },
+        },
+      ]).promise
     ).toEqual(true)
     await waitForNocks(nocks)
   })
 })
+
+describe('areAllNamespacesUnauthorized', () => {
+  it('checks each namespace individually for non-admin users', async () => {
+    nockIgnoreApiPaths()
+    const nocks = [
+      nockRBAC(adminAccess, false),
+      nockRBAC({ ...createDeployment, namespace: 'test-namespace-1' }, false),
+      nockRBAC({ ...createDeployment, namespace: 'test-namespace-2' }, true),
+    ]
+    expect(
+      await areAllNamespacesUnauthorized(Promise.resolve(createDeployment), [
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-1' },
+        },
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-2' },
+        },
+      ]).promise
+    ).toEqual(false)
+    await waitForNocks(nocks)
+  })
+  it('returns true for an empty namespace list', async () => {
+    expect(await areAllNamespacesUnauthorized(Promise.resolve(createDeployment), []).promise).toEqual(true)
+  })
+  it('returns false without checking namespaces for an admin user', async () => {
+    nockIgnoreApiPaths()
+    const nocks = [nockRBAC(adminAccess, true)]
+    expect(
+      await areAllNamespacesUnauthorized(Promise.resolve(createDeployment), [
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-1' },
+        },
+        {
+          ...(NamespaceDefinition as Pick<Namespace, 'apiVersion' | 'kind'>),
+          metadata: { name: 'test-namespace-2' },
+        },
+      ]).promise
+    ).toEqual(false)
+    await waitForNocks(nocks)
+  })
+})

frontend/src/lib/rbac-util.ts

@@ -1,7 +1,7 @@
 /* Copyright Contributors to the Open Cluster Management project */
 
+import { useEffect, useState } from 'react'
 import {
-  createShortCircuitSubjectAccessReviews,
   createSubjectAccessReview,
   createSubjectAccessReviews,
   fallbackPlural,
@@ -11,29 +11,131 @@ import {
   Namespace,
   ResourceAttributes,
 } from '../resources'
+import { useRecoilValue, useSharedAtoms } from '../shared-recoil'
 
-export async function isAnyNamespaceAuthorized(resourceAttributes: ResourceAttributes[], namespaces: Namespace[]) {
+const SELF_ACCESS_CHECK_BATCH_SIZE = 40
+
+export function isAnyNamespaceAuthorized(resourceAttributes: Promise<ResourceAttributes>, namespaces: Namespace[]) {
   const namespaceList: string[] = namespaces.map((namespace) => namespace.metadata.name!)
 
   if (namespaceList.length === 0) {
-    return false
+    return { promise: Promise.resolve(false) }
   }
-  const adminAccessRequest = await checkAdminAccess()
-  const isAdmin = adminAccessRequest?.status?.allowed ?? false
 
-  if (isAdmin) {
-    return true
+  let abortFn: () => void | undefined
+  const abort = () => {
+    abortFn?.()
   }
-  const resourceList: Array<ResourceAttributes> = []
 
-  namespaceList.forEach((namespace) => {
-    resourceList.push(...resourceAttributes.map((attribute) => ({ ...attribute, namespace })))
-  })
+  return {
+    promise: resourceAttributes.then((resourceAttributes) =>
+      checkAdminAccess().then((adminAccessRequest) => {
+        if (adminAccessRequest?.status?.allowed) {
+          return true
+        } else {
+          const resourceList: Array<ResourceAttributes> = []
+
+          namespaceList.forEach((namespace) => {
+            resourceList.push({ ...resourceAttributes, namespace })
+          })
+
+          // eslint-disable-next-line no-inner-declarations
+          async function processBatch(): Promise<boolean> {
+            const nextBatch = resourceList.splice(0, SELF_ACCESS_CHECK_BATCH_SIZE)
+            const results = nextBatch.map((resource) => {
+              return createSubjectAccessReview(resource)
+            })
+            abortFn = () => results.forEach((result) => result.abort())
+            try {
+              // short-circuit as soon as any namespace says the access is allowed
+              return await Promise.any(
+                results.map((result) =>
+                  result.promise.then((result) => {
+                    if (result.status?.allowed) {
+                      abort()
+                      return true
+                    } else {
+                      throw new Error('access not allowed')
+                    }
+                  })
+                )
+              )
+            } catch {
+              if (resourceList.length) {
+                return processBatch()
+              } else {
+                return false
+              }
+            }
+          }
+          return processBatch()
+        }
+      })
+    ),
+    abort,
+  }
+}
+
+export function areAllNamespacesUnauthorized(resourceAttributes: Promise<ResourceAttributes>, namespaces: Namespace[]) {
+  const namespaceList: string[] = namespaces.map((namespace) => namespace.metadata.name!)
+
+  if (namespaceList.length === 0) {
+    return { promise: Promise.resolve(true) }
+  }
+
+  let abortFn: () => void | undefined
+  const abort = () => {
+    abortFn?.()
+  }
+
+  return {
+    promise: resourceAttributes.then((resourceAttributes) =>
+      checkAdminAccess().then((adminAccessRequest) => {
+        if (adminAccessRequest?.status?.allowed) {
+          return false
+        } else {
+          const resourceList: Array<ResourceAttributes> = []
 
-  try {
-    return await createShortCircuitSubjectAccessReviews(resourceList).promise
-  } catch {
-    return false
+          namespaceList.forEach((namespace) => {
+            resourceList.push({ ...resourceAttributes, namespace })
+          })
+
+          // eslint-disable-next-line no-inner-declarations
+          async function processBatch(): Promise<boolean> {
+            const nextBatch = resourceList.splice(0, SELF_ACCESS_CHECK_BATCH_SIZE)
+            const results = nextBatch.map((resource) => {
+              return createSubjectAccessReview(resource)
+            })
+            abortFn = () => results.forEach((result) => result.abort())
+            try {
+              // short-circuit as soon as any namespace says the access is allowed
+              return await Promise.all(
+                results.map((result) =>
+                  result.promise.then((result) => {
+                    if (result.status && !result.status.allowed) {
+                      return true
+                    } else {
+                      abort()
+                      throw new Error('access is allowed')
+                    }
+                  })
+                )
+              ).then(() => {
+                if (resourceList.length) {
+                  return processBatch()
+                } else {
+                  return true
+                }
+              })
+            } catch {
+              return false
+            }
+          }
+          return processBatch()
+        }
+      })
+    ),
+    abort,
   }
 }
 
@@ -141,12 +243,26 @@ export function canUser(
   return selfSubjectAccessReview
 }
 
-export async function checkPermission(
-  resourceAttributes: Promise<ResourceAttributes>,
-  setStateFn: (state: boolean) => void,
-  namespaces: Namespace[]
-) {
-  setStateFn(namespaces.length ? await isAnyNamespaceAuthorized([await resourceAttributes], namespaces) : false)
+export function useIsAnyNamespaceAuthorized(resourceAttributes: Promise<ResourceAttributes>) {
+  const { namespacesState } = useSharedAtoms()
+  const namespaces = useRecoilValue(namespacesState)
+  const [someNamespaceIsAuthorized, setSomeNamespaceIsAuthorized] = useState(false)
+
+  useEffect(() => {
+    const result = (someNamespaceIsAuthorized ? areAllNamespacesUnauthorized : isAnyNamespaceAuthorized)(
+      resourceAttributes,
+      namespaces
+    )
+    result.promise.then((flipAuthorization) => {
+      if (flipAuthorization) setSomeNamespaceIsAuthorized(!someNamespaceIsAuthorized)
+    })
+
+    return () => result.abort?.()
+    // exclude someNamespaceIsAuthorized from dependency list to avoid update loop
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [resourceAttributes, namespaces])
+
+  return someNamespaceIsAuthorized
 }
 
 export function rbacResourceTestHelper(

frontend/src/resources/self-subject-access-review.ts

@@ -61,23 +61,3 @@ export function createSubjectAccessReviews(resourceAttributes: Array<ResourceAtt
     abort: () => results.forEach((result) => result.abort()),
   }
 }
-
-export function createShortCircuitSubjectAccessReviews(resourceAttributes: Array<ResourceAttributes>) {
-  const results = resourceAttributes.map((resource) => createSubjectAccessReview(resource))
-  const abort = () => results.forEach((result) => result.abort())
-  return {
-    promise: Promise.any(
-      results.map((result) =>
-        result.promise.then((result) => {
-          if (result.status?.allowed) {
-            abort()
-            return true
-          } else {
-            throw new Error('access not allowed')
-          }
-        })
-      )
-    ),
-    abort,
-  }
-}

frontend/src/routes/Applications/AdvancedConfiguration.tsx

@@ -51,7 +51,6 @@ export default function AdvancedConfiguration(props: AdvancedConfigurationPagePr
   const {
     applicationsState,
     channelsState,
-    namespacesState,
     placementDecisionsState,
     placementsState,
     placementRulesState,
@@ -64,7 +63,6 @@ export default function AdvancedConfiguration(props: AdvancedConfigurationPagePr
   const placements = useRecoilValue(placementsState)
   const placementDecisions = useRecoilValue(placementDecisionsState)
   const subscriptions = useRecoilValue(subscriptionsState)
-  const namespaces = useRecoilValue(namespacesState)
 
   const subscriptionsWithoutLocal = subscriptions.filter((subscription) => {
     return !_.endsWith(subscription.metadata.name, '-local')
@@ -816,7 +814,6 @@ export default function AdvancedConfiguration(props: AdvancedConfigurationPagePr
               table={table}
               keyFn={keyFn}
               t={t}
-              namespaces={namespaces}
               defaultToggleOption={props.defaultToggleOption}
             />
           }