Bug 1906898: Missing User RoleBindings in the Project Access Web UI #7488

debsmita1 · 2020-12-09T15:16:22Z

Fixes:
https://issues.redhat.com/browse/ODC-4989
Closes #6836

Analysis / Root cause:
The User info is added in the subjects property of the RoleBinding CR. This subjects is of type array and can have multiple users assigned the same role. So, previously while fetching the users from all the Role Bindings only the first element of subjects was being read

Solution Description:

go over all the role binding subjects to fetch users
if the role of one of the grouped subjects is changed, then it is deleted from the grouped subjects & a new role binding is created for that user

Screen shots / Gifs for design review:

Unit test coverage report:
Added new tests

Test setup:
NA

Browser conformance:

Chrome
Firefox
Safari
Edge

debsmita1 · 2020-12-09T15:16:53Z

/kind bug

openshift-ci-robot · 2020-12-11T17:36:02Z

@debsmita1: This pull request references Bugzilla bug 1906898, which is valid. The bug has been moved to the POST state.

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:

Bug 1906898: Missing User RoleBindings in the Project Access Web UI

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.

debsmita1 · 2020-12-17T17:41:46Z

/cc @andrewballantyne

jerolimov · 2021-01-06T15:26:11Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

+  const users: UserRoleBinding[] = user.subjects?.reduce((acc, obj) => {
+    acc.push({
+      roleBindingName: user.metadata.name,
+      user: obj.name,
+      role: user.roleRef.name,
+    });
+    return acc;
+  }, []);


Isn't this a map function? :)

jerolimov · 2021-01-06T15:32:11Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

-          },
-        ],
-      ]),
+    (user: RoleBinding) => (userRoleBindings = [...userRoleBindings, ...getUsersFromSubject(user)]),


Its similar to before, but the result of a map function should be used, otherwise can we switch to a .forEach here? Isn't this similar to this code?

export const getUserRoleBindings = (roleBindings: RoleBinding[]) => { const userRoleBindings: UserRoleBinding[] = []; roleBindings.forEach( (user: RoleBinding) => userRoleBindings.push(...getUsersFromSubject(user)), ); return userRoleBindings; };

(untested code). Wdyt? :)

used _.flatten here
added the test for this util in project-access-form-utils.spec.ts

jerolimov · 2021-01-06T15:40:02Z

frontend/packages/dev-console/src/components/project-access/project-access-form-submit-utils.ts

+  let groupedRole: RoleBinding;
+  _.forEach(roleBindings, (roleBinding: RoleBinding) => {
+    if (roleBinding.metadata.name === role.roleBindingName && roleBinding.subjects.length > 1) {
+      _.remove(roleBinding.subjects, (subject) => subject.name === role.user);


Hmm, not a big fan of changing input parameters in a 'get' helper function. Esp. in this case where getGroupedRole was called with some props we should avoid this.

Did you see an option that this method return new values instead of modifying the input variables?

jerolimov · 2021-01-06T15:41:44Z

...kages/dev-console/src/components/project-access/__tests__/project-access-form-utils.spec.tsx

-import { filterRoleBindings, getUserRoleBindings } from '../project-access-form-utils';
-import { Roles } from '../project-access-form-utils-types';
-import {
-  roleBindingsResourceData,
-  roleBindingsWithRequiredRolesResult,
-  roleBindingsWithRequiredRoles,
-  roleBindingsWithRequiredAttributes,
-} from './project-access-form-data';
-
-describe('Fetch required roles', () => {
-  it('should fetch the only the required rolebindings', async () => {
-    const filteredRoleBindings = filterRoleBindings(roleBindingsResourceData, Roles);
-    expect(filteredRoleBindings).toEqual(roleBindingsWithRequiredRolesResult);
-  });
-
-  it('should consider only the required attributes', async () => {
-    const userRoleBindings = getUserRoleBindings(roleBindingsWithRequiredRoles);
-    expect(userRoleBindings).toEqual(roleBindingsWithRequiredAttributes);
-  });
-});


Why remove this unit tests? The util functions still exist and are not changed, or? So I expect this tests would still run fine?

Fixed. Changed the file name to project-access-form-utils.spec.ts

jerolimov

@debsmita1 Happy new year and sorry for the late review here. I tested the PR locally and it looks like that everything works fine for the user and I like all these new types 👍

I added some small opinionated ideas and questions. Let me know what your opinion on that points.

jerolimov

(just changed review status)

andrewballantyne · 2021-01-06T16:54:46Z

frontend/packages/dev-console/src/components/project-access/project-access-form-submit-utils.ts

@@ -73,3 +76,14 @@ export const sendRoleBindingRequest = (verb: string, roles: UserRoleBinding[], r
  });
  return finalArray;
 };
+
+export const getGroupedRole = (role: UserRoleBinding, roleBindings: RoleBinding[]) => {


Your get method is mutating roleBindings. This is not an expected side-effect of get methods.

Also, as discussed before, please make use of filters and finds instead of custom running your own forEach.

used find & filter

andrewballantyne · 2021-01-06T17:37:15Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

+  const users: UserRoleBinding[] = user.subjects?.reduce((acc, obj) => {
+    acc.push({
+      roleBindingName: user.metadata.name,
+      user: obj.name,
+      role: user.roleRef.name,
+    });
+    return acc;
+  }, []);
+
+  return users;


A .reduce into an array .push is just a .map.

Suggested change

const users: UserRoleBinding[] = user.subjects?.reduce((acc, obj) => {

acc.push({

roleBindingName: user.metadata.name,

user: obj.name,

role: user.roleRef.name,

});

return acc;

}, []);

return users;

return user.subjects?.map((obj) => ({

roleBindingName: user.metadata.name,

user: obj.name,

role: user.roleRef.name,

}));

andrewballantyne · 2021-01-06T17:49:29Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

+  return users;
+};
+
+export const getUserRoleBindings = (roleBindings: RoleBinding[]) => {
  let userRoleBindings: UserRoleBinding[] = [];
  roleBindings.map(


For the record (I know this is old code) but .map should always have its response used. Ideally .map has no side-effects if possible.

andrewballantyne · 2021-01-06T17:57:23Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

 };

-export const getUserRoleBindings = (roleBindings) => {
+export const getUsersFromSubject = (user: RoleBinding) => {


Helps to declare what your returns will be when you're mutating and shifting values around. When it's a straight filter or find, and your input type is your output type, it's less of a concern.

In this case you're taking a RoleBinding and creating UserRoleBinding[]. Makes it easier to consume the method without having to read the method.

Suggested change

export const getUsersFromSubject = (user: RoleBinding) => {

export const getUsersFromSubject = (user: RoleBinding): UserRoleBinding[] => {

andrewballantyne · 2021-01-06T18:01:06Z

frontend/packages/dev-console/src/components/project-access/project-access-form-utils.ts

-          },
-        ],
-      ]),
+    (user: RoleBinding) => (userRoleBindings = [...userRoleBindings, ...getUsersFromSubject(user)]),


Suggested change

(user: RoleBinding) => (userRoleBindings = [...userRoleBindings, ...getUsersFromSubject(user)]),

return roleBindings.map((user) => getUsersFromSubject(user)).flat();

Two major things here:

Don't assign in a return statement (we should probably get a linter to stop this)

Avoid mutations in the middle of .map function calls (the purer they are the better they are)

yes, I could have used flat here to concatenate the sub-array elements. Thanks!

andrewballantyne · 2021-01-06T18:03:58Z

frontend/packages/dev-console/src/components/project-access/project-access-form-submit-utils.ts

+  let groupedRole: RoleBinding;
+  _.forEach(roleBindings, (roleBinding: RoleBinding) => {
+    if (roleBinding.metadata.name === role.roleBindingName && roleBinding.subjects.length > 1) {
+      _.remove(roleBinding.subjects, (subject) => subject.name === role.user);


Is there a reason you need to remove the matching subject?

@andrewballantyne What I am trying to do here is, if I update the role of a user(suppose A) that belongs to grouped subjects(subjects: A, B) then I am removing that user(A) from the group and sending a patch request to update the rolebinding with the user B.

andrewballantyne

/lgtm

openshift-ci-robot · 2021-01-12T13:01:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/packages/dev-console/OWNERS~~ [andrewballantyne]

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

openshift-ci · 2021-01-12T14:10:10Z

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

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`3161484`	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-12T14:16:08Z

/retest

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

openshift-ci-robot · 2021-01-12T16:46:52Z

@debsmita1: This pull request references Bugzilla bug 1906898, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1906898: Missing User RoleBindings in the Project Access Web UI

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 · 2021-01-12T16:47:13Z

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

openshift/console#7488

Bugzilla bug 1906898 has been moved to the MODIFIED state.

In response to this:

Bug 1906898: Missing User RoleBindings in the Project Access Web UI

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.

debsmita1 · 2021-02-03T06:59:13Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-03T06:59:22Z

@debsmita1: new pull request created: #8034

In response to this:

/cherry-pick release-4.6

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 the component/dev-console Related to dev-console label Dec 9, 2020

openshift-ci-robot requested review from abhinandan13jan and jerolimov December 9, 2020 15:16

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 9, 2020

debsmita1 changed the title ~~Missing User RoleBindings in the Project Access Web UI~~ Bug 1906898: Missing User RoleBindings in the Project Access Web UI Dec 11, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Dec 11, 2020

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 11, 2020

openshift-ci-robot requested a review from andrewballantyne December 17, 2020 17:41

jerolimov reviewed Jan 6, 2021

View reviewed changes

jerolimov suggested changes Jan 6, 2021

View reviewed changes

andrewballantyne suggested changes Jan 6, 2021

View reviewed changes

debsmita1 force-pushed the pa-subject-bug branch 2 times, most recently from 2aa7c88 to b9eeb61 Compare January 7, 2021 06:55

fix RoleBinding with multiple subjects

5fb9f99

debsmita1 force-pushed the pa-subject-bug branch from b9eeb61 to 5fb9f99 Compare January 7, 2021 10:15

debsmita1 requested review from jerolimov and andrewballantyne January 7, 2021 13:09

andrewballantyne approved these changes Jan 12, 2021

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 12, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021

openshift-merge-robot merged commit e799db7 into openshift:master Jan 12, 2021

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

openshift-cherrypick-robot mentioned this pull request Feb 3, 2021

[release-4.6] Bug 1924437: Missing User RoleBindings in the Project Access Web UI #8034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1906898: Missing User RoleBindings in the Project Access Web UI #7488

Bug 1906898: Missing User RoleBindings in the Project Access Web UI #7488

debsmita1 commented Dec 9, 2020 •

edited by spadgett

debsmita1 commented Dec 9, 2020

openshift-ci-robot commented Dec 11, 2020

debsmita1 commented Dec 17, 2020

jerolimov Jan 6, 2021

jerolimov Jan 6, 2021

debsmita1 Jan 7, 2021 •

edited

jerolimov Jan 6, 2021 •

edited

debsmita1 Jan 7, 2021

jerolimov Jan 6, 2021

debsmita1 Jan 7, 2021 •

edited

jerolimov left a comment

jerolimov left a comment •

edited

andrewballantyne Jan 6, 2021

debsmita1 Jan 7, 2021

andrewballantyne Jan 6, 2021

andrewballantyne Jan 6, 2021

andrewballantyne Jan 6, 2021

andrewballantyne Jan 6, 2021

debsmita1 Jan 7, 2021

andrewballantyne Jan 6, 2021

debsmita1 Jan 7, 2021

andrewballantyne left a comment

openshift-ci-robot commented Jan 12, 2021

openshift-ci bot commented Jan 12, 2021 •

edited

openshift-bot commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

debsmita1 commented Feb 3, 2021

openshift-cherrypick-robot commented Feb 3, 2021

	export const getUsersFromSubject = (user: RoleBinding) => {
	export const getUsersFromSubject = (user: RoleBinding): UserRoleBinding[] => {

	(user: RoleBinding) => (userRoleBindings = [...userRoleBindings, ...getUsersFromSubject(user)]),
	return roleBindings.map((user) => getUsersFromSubject(user)).flat();

Bug 1906898: Missing User RoleBindings in the Project Access Web UI #7488

Bug 1906898: Missing User RoleBindings in the Project Access Web UI #7488

Conversation

debsmita1 commented Dec 9, 2020 • edited by spadgett

debsmita1 commented Dec 9, 2020

openshift-ci-robot commented Dec 11, 2020

debsmita1 commented Dec 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debsmita1 Jan 7, 2021 • edited

Choose a reason for hiding this comment

jerolimov Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debsmita1 Jan 7, 2021 • edited

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

jerolimov left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 12, 2021

openshift-ci bot commented Jan 12, 2021 • edited

openshift-bot commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

debsmita1 commented Feb 3, 2021

openshift-cherrypick-robot commented Feb 3, 2021

debsmita1 commented Dec 9, 2020 •

edited by spadgett

debsmita1 Jan 7, 2021 •

edited

jerolimov Jan 6, 2021 •

edited

debsmita1 Jan 7, 2021 •

edited

jerolimov left a comment •

edited

openshift-ci bot commented Jan 12, 2021 •

edited