Adds utils for access checks (#3724)

* Adds utils for access checks * Add Route Support * Remove cache on route change * Partially deprecate the useAccessReview hook * Add pf link & remove debug code related * Simplify structure of accessReview logically * Added some clarity to teh README * Code cleanup -- syncing variable names & some extra TS * Added support for product admins having access over the deployment namespace * Fix bad logic -- poorly written & breaks tests - yay tests * Drop test related to admins only get to see settings * Removes unused import that breaks linter
opendatahub-io · Feb 12, 2025 · 05a3f3c · 05a3f3c
1 parent f9a26e5
commit 05a3f3c
Show file tree

Hide file tree

Showing 20 changed files with 595 additions and 153 deletions.
diff --git a/frontend/src/__mocks__/mockSelfSubjectAccessReview.ts b/frontend/src/__mocks__/mockSelfSubjectAccessReview.ts
@@ -5,12 +5,14 @@ type MockResourceConfigType = {
   group?: string;
   resource?: string;
   allowed?: boolean;
+  namespace?: string;
 };
 
 export const mockSelfSubjectAccessReview = ({
   verb = 'list',
   group = 'serving.kserve.io',
   resource = 'servingruntimes',
+  namespace = 'opendatahub',
   allowed = false,
 }: MockResourceConfigType): SelfSubjectAccessReviewKind => ({
   kind: 'SelfSubjectAccessReview',
@@ -20,6 +22,7 @@ export const mockSelfSubjectAccessReview = ({
       group,
       resource,
       verb,
+      namespace,
     },
   },
   status: {

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/application.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/application.cy.ts
@@ -1,9 +1,5 @@
 import { appChrome } from '~/__tests__/cypress/cypress/pages/appChrome';
-import {
-  asDisallowedUser,
-  asProductAdminUser,
-  asProjectAdminUser,
-} from '~/__tests__/cypress/cypress/utils/mockUsers';
+import { asDisallowedUser, asProductAdminUser } from '~/__tests__/cypress/cypress/utils/mockUsers';
 import { mockDashboardConfig } from '~/__mocks__';
 import { aboutDialog } from '~/__tests__/cypress/cypress/pages/aboutDialog';
 import { mockConsoleLinks } from '~/__mocks__/mockConsoleLinks';
@@ -17,12 +13,6 @@ describe('Application', () => {
     appChrome.findNavToggle().should('not.exist');
   });
 
-  it('should not show Settings nav section for non product admins', () => {
-    asProjectAdminUser();
-    appChrome.visit();
-    appChrome.findNavSection('Settings').should('not.exist');
-  });
-
   it('should show Settings nav section for cluster admins', () => {
     asProductAdminUser();
     appChrome.visit();

diff --git a/frontend/src/__tests__/cypress/cypress/utils/mockUsers.ts b/frontend/src/__tests__/cypress/cypress/utils/mockUsers.ts
@@ -116,6 +116,7 @@ const setUserConfig = (userConfig: UserConfig = {}, isAllowed = true) => {
         verb,
         group,
         resource,
+        namespace,
         allowed:
           // cluster admin can do everything
           isClusterAdmin
@@ -126,9 +127,12 @@ const setUserConfig = (userConfig: UserConfig = {}, isAllowed = true) => {
             : // only project admins can create rolebindings
             resource === 'rolebindings' && EDIT_VERBS.includes(verb)
             ? isProjectAdmin
-            : // product admins will be limited to listing resources
-            isProductAdmin
-            ? !EDIT_VERBS.includes(verb)
+            : isProductAdmin
+            ? // product admins are getting direct access to resources in the deployment namespace (but importantly, not other projects)
+              namespace === 'opendatahub'
+              ? true
+              : // product admins will be limited to listing resources
+                !EDIT_VERBS.includes(verb)
             : // everyone else can perform any action within
               !!namespace,
       }),

diff --git a/frontend/src/api/useAccessReview.ts b/frontend/src/api/useAccessReview.ts
@@ -3,14 +3,14 @@ import * as React from 'react';
 import { ProjectModel, SelfSubjectAccessReviewModel } from '~/api/models';
 import { AccessReviewResourceAttributes, SelfSubjectAccessReviewKind } from '~/k8sTypes';
 
-const checkAccess = ({
+export const checkAccess = ({
   group,
   resource,
   subresource,
   verb,
   name,
   namespace,
-}: Required<AccessReviewResourceAttributes>): Promise<SelfSubjectAccessReviewKind> => {
+}: Required<AccessReviewResourceAttributes>): Promise<boolean> => {
   // Projects are a special case. `namespace` must be set to the project name
   // even though it's a cluster-scoped resource.
   const reviewNamespace =
@@ -32,14 +32,28 @@ const checkAccess = ({
   return k8sCreateResource<SelfSubjectAccessReviewKind>({
     model: SelfSubjectAccessReviewModel,
     resource: selfSubjectAccessReview,
-  });
+  })
+    .then((result) => result.status?.allowed ?? true)
+    .catch((e) => {
+      // eslint-disable-next-line no-console
+      console.warn('SelfSubjectAccessReview failed', e);
+      return true; // if it critically fails, don't block SSAR checks; let it fail/succeed on future calls
+    });
 };
 
+/**
+ * Used for a non-cached SSAR request.
+ *
+ * Potentially obsolete -- depending on if we need a non-cached variant.
+ *
+ * @see useAccessAllowed - Cached variant
+ * @see verbModelAccess - Helper util for resourceAttributes
+ */
 export const useAccessReview = (
   resourceAttributes: AccessReviewResourceAttributes,
   shouldRunCheck = true,
 ): [boolean, boolean] => {
-  const [loaded, setLoaded] = React.useState(false);
+  const [isLoaded, setIsLoaded] = React.useState(false);
   const [isAllowed, setAllowed] = React.useState(false);
 
   const {
@@ -53,23 +67,12 @@ export const useAccessReview = (
 
   React.useEffect(() => {
     if (shouldRunCheck) {
-      checkAccess({ group, resource, subresource, verb, name, namespace })
-        .then((result) => {
-          if (result.status) {
-            setAllowed(result.status.allowed);
-          } else {
-            setAllowed(true);
-          }
-          setLoaded(true);
-        })
-        .catch((e) => {
-          // eslint-disable-next-line no-console
-          console.warn('SelfSubjectAccessReview failed', e);
-          setAllowed(true);
-          setLoaded(true);
-        });
+      checkAccess({ group, resource, subresource, verb, name, namespace }).then((allowed) => {
+        setAllowed(allowed);
+        setIsLoaded(true);
+      });
     }
   }, [group, name, namespace, resource, subresource, verb, shouldRunCheck]);
 
-  return [isAllowed, loaded];
+  return [isAllowed, isLoaded];
 };
diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx
@@ -24,6 +24,7 @@ import useStorageClasses from '~/concepts/k8s/useStorageClasses';
 import AreaContextProvider from '~/concepts/areas/AreaContext';
 import { NimContextProvider } from '~/concepts/nimServing/NIMAvailabilityContext';
 import { ModelCatalogContextProvider } from '~/concepts/modelCatalog/context/ModelCatalogContext';
+import { AccessReviewProvider } from '~/concepts/userSSAR';
 import useDevFeatureFlags from './useDevFeatureFlags';
 import Header from './Header';
 import AppRoutes from './AppRoutes';
@@ -118,44 +119,46 @@ const App: React.FC = () => {
         </Bullseye>
       ) : (
         <AppContext.Provider value={contextValue}>
-          <Page
-            className="odh-dashboard"
-            isManagedSidebar
-            isContentFilled
-            masthead={
-              <Header onNotificationsClick={() => setNotificationsOpen(!notificationsOpen)} />
-            }
-            sidebar={isAllowed ? <NavSidebar /> : undefined}
-            notificationDrawer={
-              <AppNotificationDrawer onClose={() => setNotificationsOpen(false)} />
-            }
-            isNotificationDrawerExpanded={notificationsOpen}
-            mainContainerId={DASHBOARD_MAIN_CONTAINER_ID}
-            data-testid={DASHBOARD_MAIN_CONTAINER_ID}
-            banner={
-              <DevFeatureFlagsBanner
-                dashboardConfig={dashboardConfig.spec.dashboardConfig}
-                {...devFeatureFlagsProps}
-              />
-            }
-          >
-            <ErrorBoundary>
-              <NimContextProvider>
-                {/* This will be moved to modelCatalog routes as part of RHOAIENG-18959 */}
-                <ModelCatalogContextProvider>
-                  <ProjectsContextProvider>
-                    <ModelRegistrySelectorContextProvider>
-                      <QuickStarts>
-                        <AppRoutes />
-                      </QuickStarts>
-                    </ModelRegistrySelectorContextProvider>
-                  </ProjectsContextProvider>
-                </ModelCatalogContextProvider>
-              </NimContextProvider>
-              <ToastNotifications />
-              <TelemetrySetup />
-            </ErrorBoundary>
-          </Page>
+          <AccessReviewProvider>
+            <Page
+              className="odh-dashboard"
+              isManagedSidebar
+              isContentFilled
+              masthead={
+                <Header onNotificationsClick={() => setNotificationsOpen(!notificationsOpen)} />
+              }
+              sidebar={isAllowed ? <NavSidebar /> : undefined}
+              notificationDrawer={
+                <AppNotificationDrawer onClose={() => setNotificationsOpen(false)} />
+              }
+              isNotificationDrawerExpanded={notificationsOpen}
+              mainContainerId={DASHBOARD_MAIN_CONTAINER_ID}
+              data-testid={DASHBOARD_MAIN_CONTAINER_ID}
+              banner={
+                <DevFeatureFlagsBanner
+                  dashboardConfig={dashboardConfig.spec.dashboardConfig}
+                  {...devFeatureFlagsProps}
+                />
+              }
+            >
+              <ErrorBoundary>
+                <NimContextProvider>
+                  {/* This will be moved to modelCatalog routes as part of RHOAIENG-18959 */}
+                  <ModelCatalogContextProvider>
+                    <ProjectsContextProvider>
+                      <ModelRegistrySelectorContextProvider>
+                        <QuickStarts>
+                          <AppRoutes />
+                        </QuickStarts>
+                      </ModelRegistrySelectorContextProvider>
+                    </ProjectsContextProvider>
+                  </ModelCatalogContextProvider>
+                </NimContextProvider>
+                <ToastNotifications />
+                <TelemetrySetup />
+              </ErrorBoundary>
+            </Page>
+          </AccessReviewProvider>
         </AppContext.Provider>
       )}
     </AreaContextProvider>

diff --git a/frontend/src/app/AppRoutes.tsx b/frontend/src/app/AppRoutes.tsx
@@ -149,14 +149,14 @@ const AppRoutes: React.FC = () => {
             <Route path="/notebookImages/*" element={<BYONImageRoutes />} />
             <Route path="/clusterSettings" element={<ClusterSettingsPage />} />
             <Route path="/acceleratorProfiles/*" element={<AcceleratorProfileRoutes />} />
-            <Route path="/hardwareProfiles/*" element={<HardwareProfileRoutes />} />
             <Route path="/servingRuntimes/*" element={<CustomServingRuntimeRoutes />} />
             <Route path="/connectionTypes/*" element={<ConnectionTypeRoutes />} />
             <Route path="/storageClasses/*" element={<StorageClassesPage />} />
             <Route path="/modelRegistrySettings/*" element={<ModelRegistrySettingsRoutes />} />
             <Route path="/groupSettings" element={<GroupSettingsPage />} />
           </>
         )}
+        <Route path="/hardwareProfiles/*" element={<HardwareProfileRoutes />} />
 
         <Route path="*" element={<NotFound />} />
       </Routes>

diff --git a/frontend/src/concepts/userSSAR/AccessAllowed.ts b/frontend/src/concepts/userSSAR/AccessAllowed.ts
@@ -0,0 +1,33 @@
+import React from 'react';
+import { AccessReviewResourceAttributes } from '~/k8sTypes';
+import { useAccessAllowed } from './useAccessAllowed';
+
+type AccessAllowedProps = {
+  resourceAttributes: AccessReviewResourceAttributes;
+  children: () => React.ReactNode;
+  noAccessRender?: () => React.ReactNode;
+};
+
+/**
+ * Wraps content that is lazy rendered. Uses useAccessAllowed to handle rendering content easier.
+ * Consider using verbModelAccess for resourceAttributes.
+ * @see verbModelAccess
+ * @see useAccessAllowed
+ */
+export const AccessAllowed: React.FC<AccessAllowedProps> = ({
+  resourceAttributes,
+  children,
+  noAccessRender,
+}) => {
+  const [isAllowed, isLoaded] = useAccessAllowed(resourceAttributes);
+
+  if (!isLoaded) {
+    return null;
+  }
+
+  if (!isAllowed) {
+    return noAccessRender ? noAccessRender() : null;
+  }
+
+  return children();
+};