Prevent No-Folder or root S3 model access #1512

manaswinidas · 2023-07-13T09:54:48Z

Fixes #1423

Description

Renamed "Folder Path" to "Path"
Make "Path" a required field
Add helper text to "Path" field
Add validation for "Path" field(don't allow / or /////// -> enable "Deploy" button only if "Path" is valid )

GIF(outdated):

Please refer to this comment for the latest GIF

Screen.Recording.2023-07-20.at.1.43.22.AM.mov

How Has This Been Tested?

Create a Model Server
Click on Deploy Modal
Fill out the data(Path field in both Existing and New data connection)

Test Impact

Added tests for the required "Path" field

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

manaswinidas · 2023-07-18T13:26:22Z

@vconzola @kywalker-rh @kaedward Is the text "Can either be a folder path or the path to a single model" alright?

Also, as discussed, I haven't added any inline alerts. Just enabling and disabling the "Deploy" button if the "Path" is a valid path as shown in the GIF.

Here are the other alternatives:

With inline alert and validating tick in case of wrong input:

Screen.Recording.2023-07-18.at.4.43.13.PM.mov

With a validating tick(without inline alert):

Screen.Recording.2023-07-18.at.4.45.51.PM.mov

Let me know if any of these are a better option in place of the above GIF.

...rc/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx

DaoDaoNoCode · 2023-07-18T15:25:07Z

BTW, when editing, I can still see the / path.

vconzola · 2023-07-18T15:53:44Z

@manaswinidas There are three allowable inputs, correct: (1) a model name in the root folder, (2) a folder path with no model name, or (3) a folder path + model name? If that's true, I prefer more specific helper text without the validation tick and only showing the error helper text when Deploy is selected. I find the helper text and tick changes while typing to be confusing.

I'd like @kaedward to share her opinion, but I think helper text that says something like, "This must be a model name, a folder path, or a folder path and model name." would work. If the input is invalid when the user selects deploy we would show error helper text that says something like, "Not a valid model name or folder path." in red with the error icon displayed in the field as shown here: https://www.patternfly.org/v4/components/helper-text/design-guidelines#when-to-use-helper-text.

andrewballantyne · 2023-07-18T16:57:10Z

There are three allowable inputs, correct: (1) a model name in the root folder, (2) a folder path with no model name, or (3) a folder path + model name? If that's true...

@vconzola yes that is correct

manaswinidas · 2023-07-18T17:52:57Z

BTW, when editing, I can still see the / path.

@DaoDaoNoCode Edit seems to work for me, let me know if I'm missing something

Screen.Recording.2023-07-18.at.11.05.15.PM.mov

DaoDaoNoCode · 2023-07-18T18:06:46Z

@manaswinidas What if you had a model mounted with the empty path before? Try to edit that one.

manaswinidas · 2023-07-18T18:13:21Z

@DaoDaoNoCode My change is to ensure it's not empty anymore. The "Deploy" button won't be enabled if it's empty or just has / while Deploying a model in the first place.

DaoDaoNoCode · 2023-07-18T18:14:44Z

Hmm, sounds fair. @andrewballantyne WDYT, do we need to consider the previously created models which could possibly mount on the root dir?

andrewballantyne · 2023-07-18T18:17:54Z

Hmm, sounds fair. @andrewballantyne WDYT, do we need to consider the previously created models which could possibly mount on the root dir?

I made a thread in slack -- What is important to note is we need to consider how the flow impacts existing models.

What is the downside from having an "invalid" model deployed against the root /? If it locks up the mode from accepting any other inputs, we should address that with UX.

kaedward · 2023-07-18T21:00:40Z

@andrewballantyne For the helper text, how about "Enter the path to a folder or single model."?

andrewballantyne · 2023-07-19T14:36:23Z

@andrewballantyne For the helper text, how about "Enter the path to a folder or single model."?

@kaedward Yeah, I guess we can go with that -- my only concern is technically I'd view / as a folder path heh... Could you sneak an extra couple words in there?

Enter the path to a non-root folder or single model.

Feels weird still... but it's more technically accurate

kaedward · 2023-07-19T17:50:15Z

@andrewballantyne does this sound any better?
"Enter a path to a model or folder. This path cannot point to a root folder."

andrewballantyne · 2023-07-19T19:20:23Z

Thanks @kaedward fits my mental model better... @manaswinidas please implement the last text noted by Katie.

"Enter a path to a model or folder. This path cannot point to a root folder."

manaswinidas · 2023-07-19T19:26:30Z

@andrewballantyne any other changes regarding the error helperText? I can see some comments above: #1512 (comment) or we stick to enabling the Deploy button when the input is valid?

Also, a suggestion: it will be great if we can check whether the folderPath exists from the backend side(if it's not too far-fetched).

andrewballantyne · 2023-07-19T19:43:34Z

Also, a suggestion: it will be great if we can check whether the folderPath exists from the backend side(if it's not too far-fetched).

[Tracker]: Data/Object Connections Rework #1271 🙂

@andrewballantyne any other changes regarding the error helperText? I can see some comments above: #1512 (comment) or we stick to enabling the Deploy button when the input is valid?

@manaswinidas I don't follow what you're asking here -- if you're asking for more wording, pitch the scenario to @kaedward -- if you're asking for what we should do, I think Vince answered that 🤔 Let me know how I can help!

manaswinidas · 2023-07-19T19:50:30Z

If the input is invalid when the user selects deploy we would show error helper text that says something like, "Not a valid >model name or folder path." in red with the error icon displayed in the field as shown here: >https://www.patternfly.org/v4/components/helper-text/design-guidelines#when-to-use-helper-text.

@andrewballantyne I was just confirming whether there is a need for an error helperText as shown in the comment here. I think I confused Vince with the GIFs 😅

kaedward · 2023-07-19T21:08:56Z

@manaswinidas I think error help text would be useful. Can we detect when an invalid path is entered, or only if the user enters a root folder?

manaswinidas · 2023-07-19T21:25:51Z

We are preventing inputs like / or ///// It needs to be some text that can contain / but not only /. As of now, I'm enabling the Deploy button if the Path is valid. Here's a GIF for your reference. Does this look good or it still needs an error helperText?

Screen.Recording.2023-07-20.at.1.43.22.AM.mov

vconzola · 2023-07-20T20:32:00Z

@kaedward I don't understand where/when you're suggesting that error be displayed?

manaswinidas · 2023-07-21T11:10:16Z

We have 2(or 3) options:

Show the error message while the user is typing the input. Do we still disable the "Deploy" button when the error helperText is getting displayed? Or enable the "Deploy" button anyways(even if the user is typing the wrong input) and make the user try to send the form with the wrong input, and after they click on "Deploy"- show them the error.
(My concerns in the first case: Changing the helperText to the error helperText while typing might confuse the user coz it draws attention

My concerns for the second case(disputed opinion): I've found it annoying as a user when I get an error after clicking on the "Submit" button when the form/field could have shown the error while doing it, thus saving me time.)

Disable the "Deploy" button when the user types / or ///// and enable it when the user types the correct input(the latest changes of this PR has this - refer to first comment of this PR to have a glimpse of how it looks like now)

Would you like me to send GIFs for some or all of these scenarios?

andrewballantyne · 2023-07-21T13:01:59Z

@kaedward I don't understand where/when you're suggesting that error be displayed?

@vconzola @manaswinidas @kaedward okay, let me step in here and cap this off. (hopefully)

My suggested state of this modal is this, let me know if you agree.

For the folder field...

Empty initial state => show the empty field, deploy button is disabled (invalid input)
- the red asterisk will help the user know the field is needed
- the help text of "Enter a path to a model or folder. This path cannot point to the root folder."
If the user types / we show them the error under the field "The path must not point to a root folder."
- The error goes away (and the button enables -- if the rest of the modal is good) when they continue /<any character> -- eg /a
- We stay in error if they type more than 1 / -- /////// scenario; even if they type ////<any character>
- We show the error using PF field validation (link)
If the user straight up types a non slash character, we don't show anything in the field but we enable the button (if the rest of the modal is good)
- I think it's important we don't use success validation here because we are not validating the validity of the path

Does that make sense to everyone? If not, let us take this to slack (or a meeting), as it'll likely need more back and forth.

vconzola · 2023-07-24T19:45:17Z

@andrewballantyne @manaswinidas @kaedward I'm OK with everything @andrewballantyne suggested with one exception. i think it's going to look weird if we immediately show the error as soon as the user types"/". It could cause the error message to flash briefly if they don't type the next, non-/ character quickly enough. Can we add a short (500 ms to 1 s - will probably need to play around with it to see what feels right) delay before displaying the error below the field?

manaswinidas · 2023-07-25T13:31:29Z

@kaedward @vconzola Do we have a success helperText too or just the default helperText "Enter a path to a model or folder. This path cannot point to the root folder." will do?

vconzola · 2023-07-25T13:50:28Z

@manaswinidas We don't need any success helperText - only if there's an error.

manaswinidas · 2023-07-25T15:20:21Z

@vconzola @kaedward Final GIF:

Screen.Recording.2023-07-25.at.8.49.07.PM.mov

vconzola · 2023-07-25T15:39:22Z

lgtm

andrewballantyne · 2023-07-26T13:21:26Z

@andrewballantyne @manaswinidas @kaedward I'm OK with everything @andrewballantyne suggested with one exception. i think it's going to look weird if we immediately show the error as soon as the user types"/". It could cause the error message to flash briefly if they don't type the next, non-/ character quickly enough. Can we add a short (500 ms to 1 s - will probably need to play around with it to see what feels right) delay before displaying the error below the field?

For starters, they shouldn't type the slash... the slash is in the UI -- if they do, they can see the error imo; I'm okay with the delay too. Also I imagine most people will probably not type out the value and copy paste it from the path they have in their storage.

If we expect the avg user to type the / @vconzola -- I recommend we decide that we should remove the left-of-the-field / as the original design point was for "root" and a blank field -- which naturally because of this PR is no longer the case. (we can handle that as another issue though and not hold this one up)

andrewballantyne · 2023-07-26T13:22:52Z

@kaedward @vconzola Do we have a success helperText too or just the default helperText "Enter a path to a model or folder. This path cannot point to the root folder." will do?

For what it is worth -- don't put success intentionally. We are not validating the value, we are just helping them avoid known failure points. Success would mislead the user to think we know if their folder path is accurate -- which we do not.

manaswinidas · 2023-07-26T13:28:37Z

For what it is worth -- don't put success intentionally. We are not validating the value, we are just helping them avoid known failure points. Success would mislead the user to think we know if their folder path is accurate -- which we do not.

I was reiterating the same yesterday, we don't have a success helperText and the check mark in the textbox for the same reason. Also, I've added a 1s delay, let me know if that needs to be changed. This comment has the latest GIF.

andrewballantyne · 2023-07-26T13:24:57Z

.../pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx

-  </FormGroup>
-);
+}) => {
+  type validate = 'success' | 'warning' | 'error' | 'default';


Types are always PascalCase

Also, why do you need this? Infer the data from PF.

React.ComponentProps<typeof FormGroup>['validated']

That should get you the value behind the prop of FormGroup's prop validated.

andrewballantyne · 2023-07-26T13:34:30Z

.../pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx

+    const timer = setTimeout(() => {
+      if (folderPath === '') {
+        setValidated('default');
+      } else if (folderPath === '/' || folderPath.includes('//')) {


If you get into an error -- I think you're going to not get out of it on paste... can you switch your logic around if this, else set back to default?

vconzola · 2023-07-26T15:26:05Z

For starters, they shouldn't type the slash... the slash is in the UI -- if they do, they can see the error imo; I'm okay with the delay too. Also I imagine most people will probably not type out the value and copy paste it from the path they have in their storage.

If we expect the avg user to type the / @vconzola -- I recommend we decide that we should remove the left-of-the-field / as the original design point was for "root" and a blank field -- which naturally because of this PR is no longer the case. (we can handle that as another issue though and not hold this one up)

@andrewballantyne Iirc, we had an earlier issue where we "ignore" any leading slash. And we should keep that. The idea behind adding the delay is so that the only time the user will see the error is if they enter only one or more / in the input field - which is unlikely to happen. Tl;dr - I think we're good.

@manaswinidas One other question... The example text in the input field says, "e.g., data". For this to be a valid input means the model is stored in a folder named "data". Is this confusing? Would it be more clear if we changed this to "data_folder" or removed the example altogether? Thoughts @kaedward ?

andrewballantyne · 2023-07-26T15:29:44Z

I think the design is fine the way it is... we should log another issue to handle the field if we are changing -- the placeholder and prefix / may no longer be needed in a no root design.

There are code issues and thus I cannot approve at this time. Please fix those when you get a chance @manaswinidas

andrewballantyne

Thanks for making the changes -- you really should use PascalCase for your Type variables... but this solves the real problem I saw.

/approve

~~Give me a moment to spin up my UI to test it, but should work fine.~~ - tested and lgtm but Lucas got to it first

lucferbux

/lgtm

openshift-ci · 2023-07-27T15:22:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

manaswinidas · 2023-07-27T18:02:40Z

Issue logged for changes discussed in #1512 (comment) : #1601

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jul 13, 2023

openshift-ci bot requested review from DaoDaoNoCode and lucferbux July 13, 2023 09:54

manaswinidas force-pushed the odh-1423 branch from 81e39db to 85e6798 Compare July 17, 2023 13:21

manaswinidas changed the title ~~[WIP]Prevent No-Folder or root S3 model access~~ Prevent No-Folder or root S3 model access Jul 17, 2023

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jul 17, 2023

manaswinidas force-pushed the odh-1423 branch from 85e6798 to c435e05 Compare July 18, 2023 13:13

DaoDaoNoCode requested changes Jul 18, 2023

View reviewed changes

...rc/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx Outdated Show resolved Hide resolved

openshift-ci bot assigned DaoDaoNoCode Jul 18, 2023

manaswinidas force-pushed the odh-1423 branch from c435e05 to 6a1edb7 Compare July 18, 2023 18:19

manaswinidas force-pushed the odh-1423 branch 2 times, most recently from b85c244 to d0fd60d Compare July 19, 2023 21:00

manaswinidas force-pushed the odh-1423 branch from d0fd60d to 24ef65a Compare July 25, 2023 15:18

andrewballantyne requested changes Jul 26, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Jul 26, 2023

Prevent No-Folder or root S3 model access

d902822

manaswinidas force-pushed the odh-1423 branch from 24ef65a to d902822 Compare July 27, 2023 12:29

andrewballantyne reviewed Jul 27, 2023

View reviewed changes

openshift-ci bot added the approved label Jul 27, 2023

lucferbux approved these changes Jul 27, 2023

View reviewed changes

openshift-ci bot assigned lucferbux Jul 27, 2023

openshift-ci bot added the lgtm label Jul 27, 2023

openshift-merge-robot merged commit ae1533a into opendatahub-io:main Jul 27, 2023
7 checks passed

manaswinidas mentioned this pull request Jul 27, 2023

[Bug]: Deploy model Path field enhancements #1601

Closed

1 task

manaswinidas mentioned this pull request Aug 1, 2023

Deploy model Path field enhancements #1625

Merged

7 tasks

manaswinidas mentioned this pull request Sep 28, 2023

Implement correct trimming for editing model deployment path #1880

Merged

7 tasks

manaswinidas mentioned this pull request Jan 23, 2024

Inconsistent Model Path #2369

Merged

7 tasks

Prevent No-Folder or root S3 model access #1512

Prevent No-Folder or root S3 model access #1512

Conversation

manaswinidas commented Jul 13, 2023 • edited

Description

How Has This Been Tested?

Test Impact

Request review criteria:

manaswinidas commented Jul 18, 2023

DaoDaoNoCode commented Jul 18, 2023

vconzola commented Jul 18, 2023

andrewballantyne commented Jul 18, 2023

manaswinidas commented Jul 18, 2023

DaoDaoNoCode commented Jul 18, 2023

manaswinidas commented Jul 18, 2023 • edited

DaoDaoNoCode commented Jul 18, 2023

andrewballantyne commented Jul 18, 2023

kaedward commented Jul 18, 2023

andrewballantyne commented Jul 19, 2023

kaedward commented Jul 19, 2023

andrewballantyne commented Jul 19, 2023

manaswinidas commented Jul 19, 2023 • edited

andrewballantyne commented Jul 19, 2023

manaswinidas commented Jul 19, 2023

kaedward commented Jul 19, 2023

manaswinidas commented Jul 19, 2023

vconzola commented Jul 20, 2023

manaswinidas commented Jul 21, 2023 • edited

andrewballantyne commented Jul 21, 2023 • edited

vconzola commented Jul 24, 2023

manaswinidas commented Jul 25, 2023

vconzola commented Jul 25, 2023

manaswinidas commented Jul 25, 2023

vconzola commented Jul 25, 2023

andrewballantyne commented Jul 26, 2023

andrewballantyne commented Jul 26, 2023

manaswinidas commented Jul 26, 2023

andrewballantyne Jul 26, 2023

Choose a reason for hiding this comment

andrewballantyne Jul 26, 2023

Choose a reason for hiding this comment

vconzola commented Jul 26, 2023 • edited

andrewballantyne commented Jul 26, 2023

andrewballantyne left a comment • edited

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 27, 2023

manaswinidas commented Jul 27, 2023

manaswinidas commented Jul 13, 2023 •

edited

manaswinidas commented Jul 18, 2023 •

edited

manaswinidas commented Jul 19, 2023 •

edited

manaswinidas commented Jul 21, 2023 •

edited

andrewballantyne commented Jul 21, 2023 •

edited

vconzola commented Jul 26, 2023 •

edited

andrewballantyne left a comment •

edited