Bug 1878953: Fix RBAC with upload form and golden images #7685

glekner · 2020-12-31T09:37:16Z

No description provided.

openshift-ci-robot · 2020-12-31T09:37:20Z

@glekner: This pull request references Bugzilla bug 1878953, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1878953: Fix RBAC with upload form and golden images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

glekner · 2020-12-31T09:40:25Z

/bugzilla refresh

openshift-ci-robot · 2020-12-31T09:40:34Z

@glekner: This pull request references Bugzilla bug 1878953, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rawagner · 2021-01-05T12:28:30Z

...kages/kubevirt-plugin/src/components/cdi-upload-provider/upload-pvc-form/upload-pvc-form.tsx

+  const goldenNamespaces = React.useMemo(() => {
+    return [
+      ...new Set(
+        (commonTemplates || [])
+          .map((template) => getParameterValue(template, TEMPLATE_BASE_IMAGE_NAMESPACE_PARAMETER))
+          .filter((ns) => !!ns),
+      ),
+    ];
+  }, [commonTemplates]);
+
+  const goldenNamespacesResources = React.useMemo(() => {
+    return goldenNamespaces.map((ns) => ({
+      group: DataVolumeModel.apiGroup,
+      resource: DataVolumeModel.plural,
+      verb: 'create' as K8sVerb,
+      namespace: ns,
+    }));
+  }, [goldenNamespaces]);


can be done in one useMemo

rawagner · 2021-01-05T12:55:37Z

frontend/public/components/utils/rbac.tsx

+export const useMultipleAccessReviews = (
+  multipleResourceAttributes: AccessReviewResourceAttributes[],
+  impersonate?,
+): [{ resourceAttributes: AccessReviewResourceAttributes; allowed: boolean }[], boolean] => {


I would extract this { resourceAttributes: AccessReviewResourceAttributes; allowed: boolean } as separate type as it can be reused later - ie type AccessReviewsResult = { resourceAttributes: AccessReviewResourceAttributes; allowed: boolean }; but feel free to pick a better name.

rawagner · 2021-01-05T12:55:45Z

frontend/public/components/utils/rbac.tsx

+  impersonate?,
+): [{ resourceAttributes: AccessReviewResourceAttributes; allowed: boolean }[], boolean] => {
+  const [loading, setLoading] = React.useState(true);
+  const [allowedArr, setAllowedArr] = React.useState([]);


Suggested change

const [allowedArr, setAllowedArr] = React.useState([]);

const [allowedArr, setAllowedArr] = React.useState<AccessReviewsResult[]>([]);

rawagner · 2021-01-05T12:56:38Z

frontend/public/components/utils/rbac.tsx

+    const promises = multipleResourceAttributes.map((resourceAttributes) => {
+      const {
+        group = '',
+        resource = '',
+        subresource = '',
+        verb = '' as K8sVerb,
+        name = '',
+        namespace = '',
+      } = resourceAttributes;
+      const impersonateKey = getImpersonateKey(impersonate);
+      return checkAccessInternal(
+        group,
+        resource,
+        subresource,
+        verb,
+        name,
+        namespace,
+        impersonateKey,
+      );
+    });


you can reuse checkAccess

Suggested change

const promises = multipleResourceAttributes.map((resourceAttributes) => {

const {

group = '',

resource = '',

subresource = '',

verb = '' as K8sVerb,

name = '',

namespace = '',

} = resourceAttributes;

const impersonateKey = getImpersonateKey(impersonate);

return checkAccessInternal(

group,

resource,

subresource,

verb,

name,

namespace,

impersonateKey,

);

});

const promises = multipleResourceAttributes.map((resourceAttributes) =>

checkAccess(resourceAttributes, impersonate),

);

rawagner · 2021-01-05T12:59:03Z

frontend/public/components/utils/rbac.tsx

+    Promise.all(promises)
+      .then((values) => {
+        setLoading(false);
+        const updatedAllowedArr = (values || []).map((result: SelfSubjectAccessReviewKind) => ({


I dont think you need [] fallback. You also dont need to specify result: SelfSubjectAccessReviewKind as that is correctly infered from promises.

I would also prefer to specify generic for map as AccessReviewsResult

Suggested change

const updatedAllowedArr = (values || []).map((result: SelfSubjectAccessReviewKind) => ({

const updatedAllowedArr = (values).map<AccessReviewsResult>((result) => ({

rawagner · 2021-01-05T12:59:46Z

frontend/public/components/utils/rbac.tsx

+        setLoading(false);
+      });
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [multipleResourceAttributes]);


I think we should not ignore impersonate as dependency. (and delete // eslint-disable-next-line react-hooks/exhaustive-deps above it)

rawagner · 2021-01-05T13:00:02Z

frontend/public/components/utils/rbac.tsx

@@ -124,6 +124,55 @@ export const useAccessReview2 = (
  return [isAllowed, loading];
 };

+export const useMultipleAccessReviews = (
+  multipleResourceAttributes: AccessReviewResourceAttributes[],
+  impersonate?,


Suggested change

impersonate?,

impersonate?: boolean,

rawagner · 2021-01-05T13:17:27Z

/lgtm

openshift-ci-robot · 2021-01-05T13:17:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner

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:

~~frontend/OWNERS~~ [rawagner]

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

openshift-bot · 2021-01-05T13:49:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T14:28:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T14:41:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T17:19:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T19:17:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T20:16:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T21:08:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-06T01:59:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-06T02:12:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-01-06T03:44:15Z

@glekner: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`63d8fa9`	link	`/test kubevirt-plugin`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-01-06T03:56:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-06T05:19:07Z

@glekner: All pull requests linked via external trackers have merged:

openshift/console#7685

Bugzilla bug 1878953 has been moved to the MODIFIED state.

In response to this:

Bug 1878953: Fix RBAC with upload form and golden images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Dec 31, 2020

openshift-ci-robot added component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin labels Dec 31, 2020

openshift-ci-robot requested review from irosenzw and rhamilto December 31, 2020 09:37

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 31, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 31, 2020

glekner force-pushed the fix-1878953 branch 3 times, most recently from c6d9533 to 4e0753b Compare January 5, 2021 11:50

rawagner suggested changes Jan 5, 2021

View reviewed changes

Bug 1878953: Fix RBAC with upload form and golden images

63d8fa9

glekner force-pushed the fix-1878953 branch from 4e0753b to 63d8fa9 Compare January 5, 2021 13:13

openshift-ci-robot assigned rawagner Jan 5, 2021

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 5, 2021

openshift-merge-robot merged commit 55cf4fa into openshift:master Jan 6, 2021

spadgett added this to the v4.7 milestone Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1878953: Fix RBAC with upload form and golden images #7685

Bug 1878953: Fix RBAC with upload form and golden images #7685

glekner commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

glekner commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner Jan 5, 2021

rawagner commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 6, 2021

openshift-bot commented Jan 6, 2021

openshift-ci bot commented Jan 6, 2021

openshift-bot commented Jan 6, 2021

openshift-ci-robot commented Jan 6, 2021

	const [allowedArr, setAllowedArr] = React.useState([]);
	const [allowedArr, setAllowedArr] = React.useState<AccessReviewsResult[]>([]);

	const updatedAllowedArr = (values \|\| []).map((result: SelfSubjectAccessReviewKind) => ({
	const updatedAllowedArr = (values).map<AccessReviewsResult>((result) => ({

Bug 1878953: Fix RBAC with upload form and golden images #7685

Bug 1878953: Fix RBAC with upload form and golden images #7685

Conversation

glekner commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

glekner commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner Jan 5, 2021

Choose a reason for hiding this comment

rawagner commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 6, 2021

openshift-bot commented Jan 6, 2021

openshift-ci bot commented Jan 6, 2021

openshift-bot commented Jan 6, 2021

openshift-ci-robot commented Jan 6, 2021