Fix user management permission issues #1112

msecret · 2017-06-07T01:46:58Z

Before:
The user objects in UserStore had two different data structures related to permissions:

The organization_roles / space_roles array with each role in the array
The roles object with an array of roles by org / space guid.

This is an example user object:

{
  guid: 'asdflkja',
  organization_roles: ['org_manager'],
  roles: {
    'asdfkjlsdf': ['org_manager']
  }
}

The organization/space_roles property was used for displaying what roles a user had checked off on the org and space page. This array of roles was completely replaced each time a new org or space page was loaded, which lead to some potential bugs. The roles property with org/space guid was used to check the current user's role. This was incorrectly set based on prior assumptions about how CF api returned things.

This PR changes all role logic to use the 2nd, roles with org/space guid data structure. Additionally, it now uses this roles property to get all the users that belong to an org or space. Benefits of this:

Keeps all role logic in one data structure rather then duplicate logic in different places which can easily get confusing.
Fixes some bugs related to all users having org manager roles on all orgs. Fixes potential problems with user roles being set strangely based on space and org routes loading for users.

For users. So all users have a roles field with space and org guids for each role they have.

For fetching user orgs and spaces. This was being used incorrectly for permission management so should be removed.

Because it's much more accurate of a name, as the roles are what is actually being fetched, and the data is more consistent with the org roles fetch action / route.

Should full add user to the store with correct roles.

So both space and org role actions can both use it to merge the most recent roles into the `roles` field.

Have them use the roles property for consistency, so all logic related to users and what org/space they belong to is in the same location.

Refactor the role list control to use the org/space guid for the role checks in the new data structure. Also adds a tests which hopefully will catch any broken problems later.

Because this is the correct place for them to be, not in the store.

For consistency

Same as add user roles, it's supposed to be in actions.

So it deletes the roles for the specific org and not any org.

And add tests that weren't there before.

rememberlenny · 2017-06-08T05:39:31Z

This is GREAT functionality. I didnt quite get what the two did or why the functions in CF were so different until coming in contact with these problems. Thanks for building this abstraction.

rememberlenny · 2017-06-08T05:39:54Z

static_src/actions/user_actions.js

@@ -26,9 +26,9 @@ const userActions = {
    });
  },

-  fetchSpaceUsers(spaceGuid) {
+  fetchSpaceUserRoles(spaceGuid) {


This is great. I was confused as to the naming convention broke from the tradition here.

rememberlenny · 2017-06-08T05:40:56Z

static_src/actions/user_actions.js

@@ -332,8 +318,6 @@ const userActions = {
      .then(userInfo =>
        Promise.all([
          userActions.fetchUser(userInfo.user_id),


This looks great

rememberlenny · 2017-06-08T05:42:18Z

static_src/components/users.jsx

-      initialCurrentUserAccess={ this.state.currentUserAccess }
-      initialEmpty={ this.state.empty }
-      initialLoading={ this.state.loading }
+      users={ this.state.users }


Thanks for making this port

As it's good to have the count there together, so you know the count of users was correct when you check the user was there by the list index.

rememberlenny

This looks really great. I can 100% say that you solved a very big point of confusion for me.

rememberlenny · 2017-06-08T05:45:20Z

Im gonna hit the bed, but this looks good to merge. I'd like to pull off this for my branch.

msecret · 2017-06-08T05:48:10Z

@rememberlenny let's try and work together tomorrow to merge that one in.

jcscottiii · 2017-06-08T14:30:22Z

static_src/routes.js

@@ -93,7 +93,8 @@ export function space(orgGuid, spaceGuid, next) {
  spaceActions.fetch(spaceGuid);
  serviceActions.fetchAllInstances(spaceGuid);
  userActions.changeCurrentlyViewedType('space_users');
-  userActions.fetchSpaceUsers(spaceGuid);
+  userActions.fetchOrgUsers(orgGuid);


to be consistent, is there a reason why this wasn't also renamed to be fetchOrgUserRoles?

nevermind, it already exists, but what's the difference between this and fetchOrgUserRoles

So I found that there's no CF API equivalent to fetchSpaceUsers like fetchOrgUsers. So essentially, you have to fetch org users and then fetch space user roles to assign all the users to specific roles for a space.

I think fetchOrgUsers has some more data the fetchOrgUserRoles

jcscottiii · 2017-06-08T15:09:54Z

LGTM. just had one question.

rememberlenny · 2017-06-09T17:23:58Z

Merged into #1115

msecret force-pushed the ms-fix_perms branch from 4bd00f5 to 7f0a842 Compare June 8, 2017 00:52

Marco Segreto and others added 22 commits June 7, 2017 17:52

Fix perms experimentation A.

14e1c2f

Change roles to be on roles field

01ff441

For users. So all users have a roles field with space and org guids for each role they have.

Removed unused actions

3c99787

For fetching user orgs and spaces. This was being used incorrectly for permission management so should be removed.

Rename space users fetch to roles fetch

ec213cf

Because it's much more accurate of a name, as the roles are what is actually being fetched, and the data is more consistent with the org roles fetch action / route.

Add users to store when space user roles received.

39817be

Should full add user to the store with correct roles.

Move role merger into shared function

667430e

So both space and org role actions can both use it to merge the most recent roles into the `roles` field.

Fix linting errors

c2ec0cd

Change how get all in space org work

ae7564f

Have them use the roles property for consistency, so all logic related to users and what org/space they belong to is in the same location.

Change role checks to use new data structure

aafbd47

Refactor the role list control to use the org/space guid for the role checks in the new data structure. Also adds a tests which hopefully will catch any broken problems later.

Move roles added api requests to actions

5ec1ba1

Because this is the correct place for them to be, not in the store.

Rename resourceguid to entity guid for consistentcy

0b45847

Rename resource type to entity type

52c3729

For consistency

Add tests for moving API to user actions

ed83064

Added test for space user

d18848b

Move delete user roles to actions from store

1e4a2dc

Same as add user roles, it's supposed to be in actions.

Have role delete use new role data structure

58d128e

So it deletes the roles for the specific org and not any org.

Move roles added to use new data structure

e652e29

And add tests that weren't there before.

Get rid of console statement

ee5c5c0

Fix linting issues

7e2ede2

Fix wrong route name

180d724

Add fix for when user doesn't have roles

5dc4cb3

Forgot to change call to changed name

7f0a842

msecret mentioned this pull request Jun 8, 2017

Front end utils needed to conform to new backend changes #1115

Merged

msecret requested review from jcscottiii and rememberlenny June 8, 2017 05:37

rememberlenny reviewed Jun 8, 2017

View reviewed changes

Move user check into main assertion

7ece46f

As it's good to have the count there together, so you know the count of users was correct when you check the user was there by the list index.

rememberlenny approved these changes Jun 8, 2017

View reviewed changes

jcscottiii reviewed Jun 8, 2017

View reviewed changes

rememberlenny closed this Jun 9, 2017

rememberlenny deleted the ms-fix_perms branch June 9, 2017 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix user management permission issues #1112

Fix user management permission issues #1112

msecret commented Jun 7, 2017 •

edited

Loading

rememberlenny commented Jun 8, 2017

rememberlenny Jun 8, 2017

rememberlenny Jun 8, 2017

rememberlenny Jun 8, 2017

rememberlenny left a comment

rememberlenny commented Jun 8, 2017

msecret commented Jun 8, 2017

jcscottiii Jun 8, 2017

jcscottiii Jun 8, 2017 •

edited

Loading

msecret Jun 8, 2017

msecret Jun 8, 2017

jcscottiii commented Jun 8, 2017

rememberlenny commented Jun 9, 2017

Fix user management permission issues #1112

Fix user management permission issues #1112

Conversation

msecret commented Jun 7, 2017 • edited Loading

rememberlenny commented Jun 8, 2017

rememberlenny Jun 8, 2017

Choose a reason for hiding this comment

rememberlenny Jun 8, 2017

Choose a reason for hiding this comment

rememberlenny Jun 8, 2017

Choose a reason for hiding this comment

rememberlenny left a comment

Choose a reason for hiding this comment

rememberlenny commented Jun 8, 2017

msecret commented Jun 8, 2017

jcscottiii Jun 8, 2017

Choose a reason for hiding this comment

jcscottiii Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

msecret Jun 8, 2017

Choose a reason for hiding this comment

msecret Jun 8, 2017

Choose a reason for hiding this comment

jcscottiii commented Jun 8, 2017

rememberlenny commented Jun 9, 2017

msecret commented Jun 7, 2017 •

edited

Loading

jcscottiii Jun 8, 2017 •

edited

Loading