Merge pull request #1862 from DaoDaoNoCode/upstream-issue-1582

Show alert when external route is set while token is not set for model server
opendatahub-io · Oct 5, 2023 · 101411f · 101411f
2 parents 51594ce + 0370c9b
commit 101411f
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 19 deletions.
diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts
@@ -75,3 +75,36 @@ test('Legacy Serving Runtime', async ({ page }) => {
   await expect(firstRow).toHaveClass('pf-m-expanded');
   await expect(secondRow).not.toHaveClass('pf-m-expanded');
 });
+
+test('Add server', async ({ page }) => {
+  await page.goto(
+    './iframe.html?args=&id=tests-integration-pages-modelserving-servingruntimelist--list-available-models&viewMode=story',
+  );
+
+  // wait for page to load
+  await page.waitForSelector('text=Add server');
+
+  await page.getByRole('button', { name: 'Add server', exact: true }).click();
+
+  // test that you can not submit on empty
+  await expect(page.getByRole('button', { name: 'Add', exact: true })).toBeDisabled();
+
+  // test filling in minimum required fields
+  await page.getByLabel('Model server name *').fill('Test Name');
+  await page.locator('#serving-runtime-template-selection').click();
+  await page.getByRole('menuitem', { name: 'New OVMS Server' }).click();
+  await expect(page.getByRole('button', { name: 'Add', exact: true })).toBeEnabled();
+
+  // test the if the alert is visible when route is external while token is not set
+  await expect(page.locator('#external-route-no-token-alert')).toBeHidden();
+  // external route, no token, show alert
+  await page.getByLabel('Make deployed models available through an external route').click();
+  await expect(page.locator('#alt-form-checkbox-auth')).toBeChecked();
+  await expect(page.locator('#external-route-no-token-alert')).toBeHidden();
+  // external route, set token, hide alert
+  await page.getByLabel('Require token authentication').click();
+  await expect(page.locator('#external-route-no-token-alert')).toBeVisible();
+  // internal route, set token, show alert
+  await page.getByLabel('Make deployed models available through an external route').click();
+  await expect(page.locator('#external-route-no-token-alert')).toBeHidden();
+});
diff --git a/...src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx b/...src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx
@@ -7,10 +7,12 @@ import {
   FormGroup,
   FormSection,
   Modal,
+  Popover,
   Stack,
   StackItem,
 } from '@patternfly/react-core';
 import { EitherOrNone } from '@openshift/dynamic-plugin-sdk';
+import { HelpIcon } from '@patternfly/react-icons';
 import {
   isGpuDisabled,
   useCreateServingRuntimeObject,
@@ -80,7 +82,7 @@ const ManageServingRuntimeModal: React.FC<ManageServingRuntimeModalProps> = ({
 
   const namespace = currentProject.metadata.name;
 
-  const [allowCreate, rbacLoaded] = useAccessReview({
+  const [allowCreate] = useAccessReview({
     ...accessReviewResource,
     namespace,
   });
@@ -96,7 +98,7 @@ const ManageServingRuntimeModal: React.FC<ManageServingRuntimeModalProps> = ({
   const inputValueValid = customServingRuntimesEnabled
     ? baseInputValueValid && createData.name && servingRuntimeTemplateNameValid
     : baseInputValueValid;
-  const canCreate = !actionInProgress && !tokenErrors && inputValueValid && rbacLoaded;
+  const canCreate = !actionInProgress && !tokenErrors && inputValueValid;
 
   const servingRuntimeSelected = React.useMemo(
     () =>
@@ -273,6 +275,19 @@ const ManageServingRuntimeModal: React.FC<ManageServingRuntimeModalProps> = ({
                   setAcceleratorState={setAcceleratorState}
                 />
               </StackItem>
+              {!allowCreate && (
+                <StackItem>
+                  <Popover
+                    removeFindDomNode
+                    showClose
+                    bodyContent="Model route and token authorization can only be changed by administrator users."
+                  >
+                    <Button variant="link" icon={<HelpIcon />} isInline>
+                      {"Why can't I change the model route and token authorization fields?"}
+                    </Button>
+                  </Popover>
+                </StackItem>
+              )}
               <StackItem>
                 <FormSection title="Model route" titleElement="div">
                   <FormGroup>
@@ -281,7 +296,13 @@ const ManageServingRuntimeModal: React.FC<ManageServingRuntimeModalProps> = ({
                       id="alt-form-checkbox-route"
                       name="alt-form-checkbox-route"
                       isChecked={createData.externalRoute}
-                      onChange={(check) => setCreateData('externalRoute', check)}
+                      isDisabled={!allowCreate}
+                      onChange={(check) => {
+                        setCreateData('externalRoute', check);
+                        if (check && allowCreate) {
+                          setCreateData('tokenAuth', check);
+                        }
+                      }}
                     />
                   </FormGroup>
                 </FormSection>
@@ -291,9 +312,18 @@ const ManageServingRuntimeModal: React.FC<ManageServingRuntimeModalProps> = ({
                   data={createData}
                   setData={setCreateData}
                   allowCreate={allowCreate}
-                  rbacLoaded={rbacLoaded}
                 />
               </StackItem>
+              {createData.externalRoute && !createData.tokenAuth && (
+                <StackItem>
+                  <Alert
+                    id="external-route-no-token-alert"
+                    variant="warning"
+                    isInline
+                    title="Making models available by external routes without requiring authorization can lead to security vulnerabilities."
+                  />
+                </StackItem>
+              )}
             </Stack>
           </Form>
         </StackItem>

diff --git a/...rc/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTokenSection.tsx b/...rc/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTokenSection.tsx
@@ -6,7 +6,6 @@ import {
   FormGroup,
   FormSection,
   getUniqueId,
-  Skeleton,
   Stack,
   StackItem,
 } from '@patternfly/react-core';
@@ -20,14 +19,12 @@ type ServingRuntimeTokenSectionProps = {
   data: CreatingServingRuntimeObject;
   setData: UpdateObjectAtPropAndValue<CreatingServingRuntimeObject>;
   allowCreate: boolean;
-  rbacLoaded: boolean;
 };
 
 const ServingRuntimeTokenSection: React.FC<ServingRuntimeTokenSectionProps> = ({
   data,
   setData,
   allowCreate,
-  rbacLoaded,
 }) => {
   const createNewToken = () => {
     const name = 'default-name';
@@ -43,10 +40,6 @@ const ServingRuntimeTokenSection: React.FC<ServingRuntimeTokenSectionProps> = ({
     ]);
   };
 
-  if (!rbacLoaded) {
-    return <Skeleton />;
-  }
-
   return (
     <FormSection title="Token authorization">
       <FormGroup>
@@ -65,14 +58,6 @@ const ServingRuntimeTokenSection: React.FC<ServingRuntimeTokenSectionProps> = ({
         />
       </FormGroup>
 
-      {!allowCreate && (
-        <Alert
-          variant="warning"
-          isInline
-          title="Administrator permissions in this namespace are required to generate tokens."
-        />
-      )}
-
       {data.tokenAuth && (
         <IndentSection>
           <Stack hasGutter>