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

Fetch roles and permit at login and do entity permission to permit mapping on frontend #1020

Merged
merged 4 commits into from
Mar 18, 2020

Conversation

sjd78
Copy link
Member

@sjd78 sjd78 commented May 18, 2019

Resolves: #1066

Part 1

At login, fetch all roles and permits available to the user. By fetching all of the roles and permits available to a user at login time, the set of permits based on an entity's permission set can be calculated on the front end without having to fetch the permits directly.

We can't fetch the set of permits directly when requesting an entity since the permits are on the 3rd tier down from the entity and the REST 'follow' parameter doesn't traverse that far down. See BZ1639784

Now, we can fetch an entities permissions and cross reference each permission's role with the one in the redux store and build a permit list from there without having to go back to the API.

Part 2

Refactor user permission / permit / 'canUser*' calculations. Each entity's permits can be checked without an additional REST call since they're already loaded in redux state.

Note: This is currently only applied to redux data fetched during
login. VM and Disk fetches will need to be updated in a similar
fashion in future.

The authorization (permit/canUser*) refactoring has been applied to:

  • Clusters
  • Templates
  • Vnic Profiles
  • Data Centers
  • Storage Domains

Summary of changes:

  • Refactor the redux read-only fetches to base-data.js

  • User permits are calculated from entity permissions + user groups
    (fetched at login) + roles (fetched at login). Entity permissions
    map through the user and groups to roles and permits. A user's
    authorization to access or perform a function is based on their
    net permits.

@sjd78
Copy link
Member Author

sjd78 commented May 18, 2019

@bond95, @michalskrivanek, @sgratch - I think this technique may be a viable way to reduce the number of REST calls we need to make. Currently, for every entity we want to check permits, the permissions (with roles and permits followed) are fetched directly. With this PR, I'm prefetch all the roles and each entity can just add a follows=permissions, cross reference in the fetch saga and be done.

Any thoughts on why this wouldn't be a valid way to go?

@sjd78 sjd78 marked this pull request as ready for review May 18, 2019 00:30
@michalskrivanek
Copy link
Member

it would certainly help a lot!

@sjd78
Copy link
Member Author

sjd78 commented Jun 6, 2019

@bond95 - What do you think about this approach instead of the extra calls that are currently being doing (like in #1019)?

bond95
bond95 previously requested changes Jun 13, 2019
Copy link
Contributor

@bond95 bond95 left a comment

Choose a reason for hiding this comment

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

Only one nit-pick

src/sagas/templates.js Outdated Show resolved Hide resolved
@sjd78 sjd78 force-pushed the roles-and-templates branch 2 times, most recently from 2c7a8a9 to 28ff261 Compare June 19, 2019 23:35
@sjd78 sjd78 dismissed bond95’s stale review June 19, 2019 23:35

Removed Api.templateToInternal since it is no longer used.

@sjd78 sjd78 requested a review from bond95 June 19, 2019 23:36
@sjd78 sjd78 mentioned this pull request Jun 20, 2019
@sjd78 sjd78 force-pushed the roles-and-templates branch 4 times, most recently from a8f7612 to b3967b6 Compare June 28, 2019 23:46
@sjd78 sjd78 changed the title Fetch roles and permit at login to allow entity permission to permit mapping on frontend, apply to templates Fetch roles and permit at login and do entity permission to permit mapping on frontend Jun 28, 2019
bond95
bond95 previously requested changes Jul 3, 2019
src/actions/roles.js Show resolved Hide resolved
src/ovirtapi/transform.js Outdated Show resolved Hide resolved
src/sagas/storageDomains.js Show resolved Hide resolved
src/sagas/storageDomains.js Outdated Show resolved Hide resolved
src/sagas/storageDomains.js Outdated Show resolved Hide resolved
src/sagas/utils.js Outdated Show resolved Hide resolved
@sjd78 sjd78 dismissed bond95’s stale review July 8, 2019 18:32

Addressed review comments.

@sjd78
Copy link
Member Author

sjd78 commented Jul 8, 2019

What kind of difference does this change make? Hitting the brq-dev engine, I have the following numbers from initial app request to the APP_CONFIGURED action:

Code Base XHR Req Count XHR Size Load time
master 46 99.7 KB 16.4s
this pr 24 (~48% reduction) 44.1 KB (~56% reduction) 11.3s (~31% reduction)

The request count and size different is due to not requesting each entities's permits explicitly. The number will vary depending on how many things each user has access to in their environment. Given that caveat, for admin @ brq-dev, it is a 48% request count reduction, 56% request size reduction and 69% of the original time taken. Decent changes!

@sjd78 sjd78 requested a review from bond95 July 9, 2019 18:04
bond95
bond95 previously requested changes Jul 18, 2019
src/sagas/storageDomains.js Outdated Show resolved Hide resolved
src/sagas/storageDomains.js Outdated Show resolved Hide resolved
@sjd78 sjd78 dismissed bond95’s stale review July 18, 2019 23:49

Broke up fetchIsoFiles into small functions.

@sjd78 sjd78 requested a review from bond95 July 18, 2019 23:49
Copy link
Contributor

@bond95 bond95 left a comment

Choose a reason for hiding this comment

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

LGTM

@sjd78 sjd78 added the Status: ON_QA status ON_QA (currently being tested) label Jul 25, 2019
@sjd78 sjd78 force-pushed the roles-and-templates branch 2 times, most recently from 9eb319f to 40fcc20 Compare August 2, 2019 14:21
@sjd78 sjd78 added this to the 1.6.0 (ovirt 4.4) milestone Aug 6, 2019
@sjd78 sjd78 force-pushed the roles-and-templates branch 2 times, most recently from 90b55b8 to 6300095 Compare August 29, 2019 04:32
@sjd78
Copy link
Member Author

sjd78 commented Sep 7, 2019

(rebased)

@sjd78
Copy link
Member Author

sjd78 commented Sep 23, 2019

(rebased)

@sjd78
Copy link
Member Author

sjd78 commented Nov 19, 2019

(rebased)

@pnovotny
Copy link

pnovotny commented Jan 6, 2020

ci build please

@sjd78
Copy link
Member Author

sjd78 commented Mar 1, 2020

Rebased and is working again on top of current master.
(@sgratch)

@sjd78
Copy link
Member Author

sjd78 commented Mar 1, 2020

@sgratch, @michalskrivanek - CI is failing here because nodejs-modules isn't build correctly for el7 and fc30 on a my pre-seed patch https://gerrit.ovirt.org/#/c/107309/. Once nodejs-modules actually can run a clean re-merge, CI will work again.

@sjd78
Copy link
Member Author

sjd78 commented Mar 3, 2020

ci test please

By fetching all of the roles, with permits, that are available to a
user at login time, the set of permits available to the user based on
their permission set can be calculated on the front end without having
to fetch the permits directly.

We can't fetch the set of permits directly when requesting an entity
since the permits are on the 3rd tier down from the entity and the
REST 'follow' parameter doesn't traverse that far down. [1]

Now, we can fetch an entities permissions and cross reference each
permission's role with the one in the redux store and build a permit
list from there without having to go back to the API.

[1] - https://bugzilla.redhat.com/show_bug.cgi?id=1639784
Note: This is currently only applied to redux data fetched during
      login.  VM and Disk fetches will need to be updated in a similar
      fashion in future.

The authorization (permit/canUser*) refactoring has been applied to:
  - Clusters
  - Templates
  - Vnic Profiles
  - Data Centers
  - Storage Domains

Summary of changes:
  - Refactor the redux read-only fetches to `base-data.js`

  - User permits are calculated from entity permissions + user groups
    (fetched at login) + roles (fetched at login).  Entity permissions
    map through the user and groups to roles and permits.   A user's
    authorization to access or perform a function is based on their
    net permits.

  - API calls updated as necessary to fetch "follow=permissions"

  - Remove unused permit fetching code
  - Refactored login and template handling
  - Removed template sagas, reducers and constants no longer needed
  - Permission to permit mapping function now takes the entity
    itself so if permissions are missing, sagas will not fail. No
    permissions == no permits.
@pnovotny
Copy link

ci build please

@pnovotny
Copy link

LGTM

@sjd78 sjd78 merged commit bcb6072 into oVirt:master Mar 18, 2020
@sjd78 sjd78 deleted the roles-and-templates branch May 5, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Flag: Needs QE needs testing by QE team before merging PR and closing issue Status: ON_QA status ON_QA (currently being tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFE] Perform permission to permit mapping on front end
4 participants