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

Check user permissions before promoting data science projects #991

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Mar 6, 2023

Closes #963

Description

We can use SelfSubjectReviewAccess to check the permissions of a specific user to some resources. For this issue, we need to check the update permission to the given project, but everything needs to be done at the backend so we need to call passThrough and the k8s API endpoint directly before the service account do the labels patching.

How Has This Been Tested?

Prerequisite: You must have 2 accounts on the cluster, 1 as the cluster admin (ADMIN) and one as the regular user (USER).

  1. Deploy the image of this PR pr-991 to your dashboard pod
  2. Create a regular project (non-DSG project) using the OpenShift console or the oc command with the ADMIN account
  3. Login to the dashboard with both the ADMIN account and USER account and go to the URL ${dashboardURL}/api/namespaces/${projectYouCreated}/1, this command will patch a label and promote the project to a model serving project
  4. Check the result of these 2 accounts, for ADMIN, it should return { applied: true }, and for the USER account, it should return a 403 error
  5. Add a RoleBinding to the namespace, the role is admin and the subject is the regular user, which gives the regular user admin permission to the project
  6. Use the USER account to request that URL again, you will see { applied: true }
  7. Delete the RoleBinding and create a new RoleBinding but change the role to edit
  8. Repeat step 6 and you will find it returns a 403 error again
  9. Use the USER account to create a new project as in step 2, and test the permission in the dashboard as in steps 3 and 4

Follow the test instructions in the last section, you will see this if you have admin permission for the project you are trying to promote:
Screenshot 2023-03-09 at 4 22 40 PM

And the project will be promoted:
Screenshot 2023-03-09 at 4 26 02 PM

If you don't have permission:
Screenshot 2023-03-09 at 4 25 11 PM

Test Impact

N/A, this needs to be tested on the cluster since we need the service account.

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • 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.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@openshift-ci openshift-ci bot requested review from Gkrumbach07 and lucferbux March 6, 2023 05:45
@DaoDaoNoCode
Copy link
Member Author

/hold Waiting for an image built to deploy and test.

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Mar 6, 2023
@DaoDaoNoCode DaoDaoNoCode changed the title Use access token to promote projects [WIP] Use access token to promote projects Mar 6, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Mar 6, 2023
@DaoDaoNoCode DaoDaoNoCode changed the title [WIP] Use access token to promote projects Check user permissions before promoting data science projects Mar 9, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Mar 9, 2023
@DaoDaoNoCode
Copy link
Member Author

/unhold Everything is updated and ready for review.

@DaoDaoNoCode DaoDaoNoCode removed the do-not-merge/hold This PR is hold for some reason label Mar 9, 2023
backend/src/routes/api/k8s/pass-through.ts Outdated Show resolved Hide resolved
backend/src/routes/api/namespaces/namespaceUtils.ts Outdated Show resolved Hide resolved
backend/src/routes/api/namespaces/namespaceUtils.ts Outdated Show resolved Hide resolved
backend/src/routes/api/k8s/pass-through.ts Outdated Show resolved Hide resolved
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

We need to look at our linter again -- seems just omitting types is still allowed.

backend/src/routes/api/k8s/pass-through.ts Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Mar 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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 Mar 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit 3790cbb into opendatahub-io:main Mar 9, 2023
lucferbux pushed a commit to lucferbux/odh-dashboard that referenced this pull request Mar 13, 2023
…tahub-io#991)

* Use access token to promote projects

* update to use access token

* Use pass-through API and selfSubjectAccessReview to check the user permissions

* lint

* address comments
bartoszmajsak pushed a commit to maistra/odh-dashboard that referenced this pull request Mar 30, 2023
…tahub-io#991)

* Use access token to promote projects

* update to use access token

* Use pass-through API and selfSubjectAccessReview to check the user permissions

* lint

* address comments
strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023
…tahub-io#991)

* Use access token to promote projects

* update to use access token

* Use pass-through API and selfSubjectAccessReview to check the user permissions

* lint

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Improve Namespace Endpoint
4 participants