Improve username logic and add navbar elements (#641)

* Enable kubeflow user id header from endpoint Signed-off-by: lucferbux <[email protected]> * Add fake namespace selector and user in navbar Signed-off-by: lucferbux <[email protected]> * Handle user config Signed-off-by: lucferbux <[email protected]> * Add user endpoint Signed-off-by: lucferbux <[email protected]> * Fix tests Signed-off-by: lucferbux <[email protected]> --------- Signed-off-by: lucferbux <[email protected]>
kubeflow · Dec 13, 2024 · 2fb4d26 · 2fb4d26
1 parent e599cda
commit 2fb4d26
Show file tree

Hide file tree

Showing 16 changed files with 223 additions and 66 deletions.
diff --git a/clients/ui/api/openapi/mod-arch.yaml b/clients/ui/api/openapi/mod-arch.yaml
@@ -25,13 +25,15 @@ paths:
       operationId: healthcheck
       summary: HealthCheck
       description: HealthCheck endpoint.
-  /api/v1/config:
-    summary: Path used to manage Configuration information regarding the UI.
+  /api/v1/user:
+    summary: Path used to Retrieve a user based on the header.
     description: >-
       The REST endpoint/path used pass all the config information needed for the UI.
     get:
       tags:
         - K8SOperation
+      parameters:
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ConfigResponse"
@@ -49,6 +51,8 @@ paths:
     get:
       tags:
         - K8SOperation
+      parameters:
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ModelRegistryRespone"
@@ -68,6 +72,7 @@ paths:
         - ModelRegistryService
       parameters:
         - $ref: "#/components/parameters/modelRegistryName"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ModelVersionResponse"
@@ -112,6 +117,7 @@ paths:
       description: Updates an existing `ModelVersion`.
     parameters:
       - $ref: "#/components/parameters/modelRegistryName"
+      - $ref: "#/components/parameters/kubeflowUserId"
       - name: modelversionId
         description: A unique identifier for a `ModelVersion`.
         schema:
@@ -131,6 +137,7 @@ paths:
         - $ref: "#/components/parameters/orderBy"
         - $ref: "#/components/parameters/sortOrder"
         - $ref: "#/components/parameters/nextPageToken"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/RegisteredModelListResponse"
@@ -159,6 +166,7 @@ paths:
         - ModelRegistryService
       parameters:
         - $ref: "#/components/parameters/modelRegistryName"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "201":
           $ref: "#/components/responses/RegisteredModelResponse"
@@ -180,6 +188,7 @@ paths:
         - ModelRegistryService
       parameters:
         - $ref: "#/components/parameters/modelRegistryName"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/RegisteredModelResponse"
@@ -224,6 +233,7 @@ paths:
       description: Updates an existing `RegisteredModel`.
     parameters:
       - $ref: "#/components/parameters/modelRegistryName"
+      - $ref: "#/components/parameters/kubeflowUserId"
       - name: registeredmodelId
         description: A unique identifier for a `RegisteredModel`.
         schema:
@@ -245,6 +255,7 @@ paths:
         - $ref: "#/components/parameters/orderBy"
         - $ref: "#/components/parameters/sortOrder"
         - $ref: "#/components/parameters/nextPageToken"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ArtifactListResponse"
@@ -274,6 +285,7 @@ paths:
         - ModelRegistryService
       parameters:
         - $ref: "#/components/parameters/modelRegistryName"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ArtifactResponse"
@@ -311,6 +323,7 @@ paths:
         - $ref: "#/components/parameters/orderBy"
         - $ref: "#/components/parameters/sortOrder"
         - $ref: "#/components/parameters/nextPageToken"
+        - $ref: "#/components/parameters/kubeflowUserId"
       responses:
         "200":
           $ref: "#/components/responses/ModelVersionListResponse"
@@ -355,6 +368,7 @@ paths:
       description: Creates a new instance of a `ModelVersion`.
     parameters:
       - $ref: "#/components/parameters/modelRegistryName"
+      - $ref: "#/components/parameters/kubeflowUserId"
       - name: registeredmodelId
         description: A unique identifier for a `RegisteredModel`.
         schema:
@@ -365,12 +379,16 @@ components:
   schemas:
     Config:
       required:
-        - username
+        - userId
+        - clusterAdmin
       type: object
       properties:
-        username:
+        userId:
           type: string
-          example: username-1
+          example: [email protected]
+        clusterAdmin:
+          type: boolean
+          example: true
     ModelRegistry:
       required:
         - name
@@ -1193,6 +1211,12 @@ components:
                 $ref: "#/components/schemas/ServeModel"
       description: A response containing a `ServeModel` entity.
   parameters:
+    kubeflowUserId:
+      in: header
+      name: kubeflow-userid
+      schema:
+        type: string
+      required: true
     modelRegistryName:
       name: modelRegistryName
       description: Name of the Model Registry selected.

diff --git a/clients/ui/bff/internal/models/health_check.go b/clients/ui/bff/internal/models/health_check.go
@@ -7,5 +7,5 @@ type SystemInfo struct {
 type HealthCheckModel struct {
 	Status     string     `json:"status"`
 	SystemInfo SystemInfo `json:"system_info"`
-	UserID     string     `json:"user-id"`
+	UserID     string     `json:"userId"`
 }
diff --git a/clients/ui/bff/internal/models/user.go b/clients/ui/bff/internal/models/user.go
@@ -1,6 +1,6 @@
 package models
 
 type User struct {
-	UserID       string `json:"user-id"`
-	ClusterAdmin bool   `json:"cluster-admin"`
+	UserID       string `json:"userId"`
+	ClusterAdmin bool   `json:"clusterAdmin"`
 }
diff --git a/clients/ui/frontend/src/__mocks__/mockUserSettings.ts b/clients/ui/frontend/src/__mocks__/mockUserSettings.ts
@@ -0,0 +1,14 @@
+import { UserSettings } from '~/shared/types';
+
+type MockUserSettingsType = {
+  userId?: string;
+  clusterAdmin?: boolean;
+};
+
+export const mockUserSettings = ({
+  userId = '[email protected]',
+  clusterAdmin = true,
+}: MockUserSettingsType): UserSettings => ({
+  userId,
+  clusterAdmin,
+});
diff --git a/clients/ui/frontend/src/__tests__/cypress/cypress/support/commands/api.ts b/clients/ui/frontend/src/__tests__/cypress/cypress/support/commands/api.ts
@@ -9,6 +9,7 @@ import type {
   RegisteredModel,
   RegisteredModelList,
 } from '~/app/types';
+import type { UserSettings } from '~/shared/types';
 
 const MODEL_REGISTRY_API_VERSION = 'v1';
 export { MODEL_REGISTRY_API_VERSION };
@@ -102,6 +103,11 @@ declare global {
           type: 'GET /api/:apiVersion/model_registry',
           options: { path: { apiVersion: string } },
           response: ApiResponse<ModelRegistry[]>,
+        ) => Cypress.Chainable<null>) &
+        ((
+          type: 'GET /api/:apiVersion/user',
+          options: { path: { apiVersion: string } },
+          response: ApiResponse<UserSettings>,
         ) => Cypress.Chainable<null>);
     }
   }

diff --git a/clients/ui/frontend/src/__tests__/cypress/cypress/support/e2e.ts b/clients/ui/frontend/src/__tests__/cypress/cypress/support/e2e.ts
@@ -15,8 +15,10 @@
 
 import chaiSubset from 'chai-subset';
 import '@cypress/code-coverage/support';
+import { mockUserSettings } from '~/__mocks__/mockUserSettings';
 import 'cypress-mochawesome-reporter/register';
 import './commands';
+import { MODEL_REGISTRY_API_VERSION } from './commands/api';
 
 chai.use(chaiSubset);
 
@@ -28,5 +30,15 @@ beforeEach(() => {
   if (Cypress.env('MOCK')) {
     // fallback: return 404 for all api requests
     cy.intercept({ pathname: '/api/**' }, { statusCode: 404 });
+
+    cy.interceptApi(
+      'GET /api/:apiVersion/user',
+      {
+        path: {
+          apiVersion: MODEL_REGISTRY_API_VERSION,
+        },
+      },
+      mockUserSettings({}),
+    );
   }
 });
diff --git a/clients/ui/frontend/src/app/App.tsx b/clients/ui/frontend/src/app/App.tsx
@@ -5,9 +5,6 @@ import {
   Alert,
   Bullseye,
   Button,
-  Masthead,
-  MastheadContent,
-  MastheadMain,
   Page,
   PageSection,
   Spinner,
@@ -16,11 +13,12 @@ import {
 } from '@patternfly/react-core';
 import ToastNotifications from '~/shared/components/ToastNotifications';
 import { useSettings } from '~/shared/hooks/useSettings';
-import { isMUITheme, Theme } from '~/shared/utilities/const';
+import { isMUITheme, Theme, USER_ID } from '~/shared/utilities/const';
 import NavSidebar from './NavSidebar';
 import AppRoutes from './AppRoutes';
 import { AppContext } from './AppContext';
 import { ModelRegistrySelectorContextProvider } from './context/ModelRegistrySelectorContext';
+import NavBar from './NavBar';
 
 const App: React.FC = () => {
   const {
@@ -30,6 +28,8 @@ const App: React.FC = () => {
     loadError: configError,
   } = useSettings();
 
+  const username = userSettings?.userId;
+
   React.useEffect(() => {
     // Apply the theme based on the value of STYLE_THEME
     if (isMUITheme()) {
@@ -39,6 +39,16 @@ const App: React.FC = () => {
     }
   }, []);
 
+  React.useEffect(() => {
+    // Add the user to localStorage if in PoC
+    // TODO: [Env Handling] Just add this logic in PoC mode
+    if (username) {
+      localStorage.setItem(USER_ID, username);
+    } else {
+      localStorage.removeItem(USER_ID);
+    }
+  }, [username]);
+
   const contextValue = React.useMemo(
     () =>
       configSettings && userSettings
@@ -82,15 +92,6 @@ const App: React.FC = () => {
   // Waiting on the API to finish
   const loading = !configLoaded || !userSettings || !configSettings || !contextValue;
 
-  const masthead = (
-    <Masthead>
-      <MastheadMain />
-      <MastheadContent>
-        {/* TODO: [Auth-enablement] Add logout and user status once we enable itNavigates to register page from table toolbar */}
-      </MastheadContent>
-    </Masthead>
-  );
-
   return loading ? (
     <Bullseye>
       <Spinner />
@@ -99,7 +100,14 @@ const App: React.FC = () => {
     <AppContext.Provider value={contextValue}>
       <Page
         mainContainerId="primary-app-container"
-        masthead={masthead}
+        masthead={
+          <NavBar
+            username={username}
+            onLogout={() => {
+              //TODO: [Auth-enablement] Logout when auth is enabled
+            }}
+          />
+        }
         isManagedSidebar
         sidebar={<NavSidebar />}
       >

diff --git a/clients/ui/frontend/src/app/AppRoutes.tsx b/clients/ui/frontend/src/app/AppRoutes.tsx
@@ -3,6 +3,7 @@ import { Navigate, Route, Routes } from 'react-router-dom';
 import { NotFound } from './pages/notFound/NotFound';
 import ModelRegistrySettingsRoutes from './pages/settings/ModelRegistrySettingsRoutes';
 import ModelRegistryRoutes from './pages/modelRegistry/ModelRegistryRoutes';
+import { useAppContext } from './AppContext';
 
 export const isNavDataGroup = (navItem: NavDataItem): navItem is NavDataGroup =>
   'children' in navItem;
@@ -22,12 +23,9 @@ export type NavDataGroup = NavDataCommon & {
 type NavDataItem = NavDataHref | NavDataGroup;
 
 export const useAdminSettings = (): NavDataItem[] => {
-  // get auth access for example set admin as true
-  const isAdmin = true; //this should be a call to getting auth / role access
+  const { clusterAdmin } = useAppContext().user;
 
-  // TODO: [Auth-enablement] Remove the linter skip when we implement authentication
-  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-  if (!isAdmin) {
+  if (!clusterAdmin) {
     return [];
   }
 
@@ -48,20 +46,16 @@ export const useNavData = (): NavDataItem[] => [
 ];
 
 const AppRoutes: React.FC = () => {
-  const isAdmin = true;
+  const { clusterAdmin } = useAppContext().user;
 
   return (
     <Routes>
       <Route path="/" element={<Navigate to="/modelRegistry" replace />} />
       <Route path="/modelRegistry/*" element={<ModelRegistryRoutes />} />
       <Route path="*" element={<NotFound />} />
-      {
-        // TODO: [Auth-enablement] Remove the linter skip when we implement authentication
-        // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-        isAdmin && (
-          <Route path="/modelRegistrySettings/*" element={<ModelRegistrySettingsRoutes />} />
-        )
-      }
+      {clusterAdmin && (
+        <Route path="/modelRegistrySettings/*" element={<ModelRegistrySettingsRoutes />} />
+      )}
     </Routes>
   );
 };