add warn to accessReview for null

[atiratree]

No description provided.

[vojtechszocs]

/approve

[vojtechszocs]

/lgtm

[spadgett]

/hold

[spadgett]

Should we be returning no access review in this case? Or is the intent to check if you can perform the action on all resources of this kind in the cluster? In what case is this occurring?

[spadgett]

I'm concerned this is going to mask bugs where we're not passing the object when we mean to. Note that for normal users this will almost always disable the action (and it would be really easy to miss if testing as kubeadmin). If we want to check create, we should have a different utility imo. For namespaced resources, you would want to pass the create namespace as well.

[atiratree]

I see, we need another way for testing the permission.

I would still prefer to use this selector though and use some other way to test for nulls than crashing the UI.

Closing the PR

[spadgett]

We can add a null check, but we should be permissive by default if the object is not set. Maybe logging a warning is enough. But really this should never be null if it's a kebab action.

[atiratree]

ok, I added the null check.

it can happen quite easily to miss the null check if not used like this: https://github.com/openshift/console/blob/e1df91c790eeb1b2ca3c7efc8511a4ca63058cd9/frontend/packages/metal3-plugin/src/components/host-menu-actions.tsx#L52

some cases similar to these could be easily missed in the future

[spadgett]

/approve cancel
/lgtm cancel

[atiratree]

reopening and adding the null check

[spadgett]

return null if the obj is null

[spadgett]

If you don't return null here, it will make an entirely different, unintended access review check. That is my original concern with the change :)

[atiratree]

ah sorry, my bad. Fixed

[spadgett]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, suomiy, vojtechszocs

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [spadgett,vojtechszocs]

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

[atiratree]

fixed console lint

[spadgett]

/hold cancel

[atiratree]

/retest