Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show alert when external route is set while token is not set for model server #1862

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Sep 25, 2023

Closes #1582
Closes #1581

Description

Added the alert as the description in the issue. One thing that needs to be noted is that the regular user with edit permission cannot enable the token, so the alert will show as soon as the user enables the external route.
Found issue #1581 and also updated it here because they are tightly related, the user with edit permission cannot change the external route enablement anymore, and added a popover to explain that.

As the project admin:
Screenshot 2023-09-25 at 3 55 38 PM

As the project user with edit access:
Screenshot 2023-10-02 at 1 37 13 PM

How Has This Been Tested?

  1. Go to the project details page, try to add a model server
  2. In the adding modal, check to enable the external route
  3. Make sure the alert is shown
  4. Check to enable token
  5. Make sure the alert is gone
  6. Try to impersonate as a user with the edit permission
  7. Open the add server modal to see both fields are disabled and check the popover content in the link button above

Test Impact

Added integration test to test all the situations where the alert is visible/hidden.

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 (find relevant UX in the SMEs section).

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

@DaoDaoNoCode
Copy link
Member Author

@vconzola Can you check the UI changes above?

@vconzola
Copy link

@DaoDaoNoCode Just to be clear, for the project admin (first screenshot) the Require token auth checkbox is auto-selected when the user selects to enable external routes, and the warning is only displayed when the Require token auth checkbox is then unchecked. Is that correct?

For the second use case, project user with edit permissions, looks correct. As soon as the Enable external routes checkbox is checked, then display the warnings.

@DaoDaoNoCode
Copy link
Member Author

@DaoDaoNoCode Just to be clear, for the project admin (first screenshot) the Require token auth checkbox is auto-selected when the user selects to enable external routes, and the warning is only displayed when the Require token auth checkbox is then unchecked. Is that correct?

@vconzola Currently it's not, the required token will not be auto-checked together along with the external route checked, if that's the case you are expecting, I could make the change.

@DaoDaoNoCode
Copy link
Member Author

@vconzola Here is the latest flow:

Screen.Recording.2023-09-26.at.10.18.31.AM.mov

@vconzola
Copy link

@DaoDaoNoCode The video shows the flow as I would expect it. So lgtm.

@lucferbux
Copy link
Contributor

@DaoDaoNoCode The video shows the flow as I would expect it. So lgtm.

Yes, the message should appear regardless wether the user is admin or not, cause the security risk (non authenticated requests) is there for everyone, so yes, this seems great, I'll take a look later to review it.

@andrewballantyne
Copy link
Member

/hold

@DaoDaoNoCode the issue is a story -- you'll want to target this to the feature branch

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Sep 29, 2023
@DaoDaoNoCode DaoDaoNoCode changed the base branch from main to f/model-serving October 2, 2023 13:41
@DaoDaoNoCode
Copy link
Member Author

/unhold
Changed the target to the feature branch.

@openshift-ci openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Oct 2, 2023
@DaoDaoNoCode
Copy link
Member Author

@vconzola Hi, I just noticed that this PR can also solve the issue in #1581, can you check the second screenshot in the description? (As the project user with edit access)

@vconzola
Copy link

vconzola commented Oct 3, 2023

@DaoDaoNoCode I think the changes (warning and popover) look fine. But I do have a couple questions:

  1. Does it ever make sense to have token auth checked when external routes is not checked? If not, then do we need to change the interaction of those two checkboxes so that the state shown can never exist?
  2. When a user with Edit permissions adds a model server, are the Route and Token fields checked by default or unchecked by default? I thought they were unchecked, but I'm not sure. Not sure it matters for this PR; I'm just curious.

@DaoDaoNoCode
Copy link
Member Author

@vconzola Good question.

  1. I would like @lucferbux to answer this question because I am not very sure about it
  2. The fields are unchecked by default

@lucferbux
Copy link
Contributor

  1. Does it ever make sense to have token auth checked when external routes is not checked? If not, then do we need to change the interaction of those two checkboxes so that the state shown can never exist?

Yes, we had this discussion ages ago in a UX Meeting, maybe an admin/project owner wants to first enable the tokens to set up the model server, but keep the route internal AND after some tests or something make the model server public

  1. When a user with Edit permissions adds a model server, are the Route and Token fields checked by default or unchecked by default? I thought they were unchecked, but I'm not sure. Not sure it matters for this PR; I'm just curious.

They are unchecked by default
Screenshot 2023-10-04 at 17 38 59

@DaoDaoNoCode
Copy link
Member Author

Great, then I think no more changes need to be made to this PR and it's ready to get reviewed!

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@openshift-ci openshift-ci bot added the approved label Oct 5, 2023
@openshift-ci openshift-ci bot merged commit 101411f into opendatahub-io:f/model-serving Oct 5, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants