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

Fix/permissions PXP-4524 #610

Merged
merged 6 commits into from
Nov 1, 2019
Merged

Fix/permissions PXP-4524 #610

merged 6 commits into from
Nov 1, 2019

Conversation

vpsx
Copy link
Contributor

@vpsx vpsx commented Oct 25, 2019

This PR makes the profile page show authz info from fence or arborist or both or neither, depending on the config. Additionally it fixes the file page's download button. See details below.

For download button, here is why we are throwing out the Fence authz checks:

  1. They appear to have been ad hoc in the first place
  2. They are broken in some cases in commons using centralized auth, whenever fence's userinfo project_access is no longer in sync with Arborist authz (e.g. synapse).
  3. To fall back on the Fence checks if no centralized auth would have required a third new "useArboristAuthz" config var and there was already pushback from the team against more config
  4. We are transitioning to centralized auth anyway

New Features

  • add config vars showArboristAuthzOnProfile and showFenceAuthzOnProfile
  • make profile page show authz info from fence/arborist/both/neither based on configs
  • make file page 'download' button toggle according to arborist authz

Breaking Changes

  • file page 'download' button will always render if useArboristUI is false (it no longer respects Fence authz). If useArboristUI is true, it will render if user has read-storage. Previously, it checked user.project_access (from Fence userinfo) and rendered if user had read-storage.

@mfshao
Copy link
Collaborator

mfshao commented Oct 25, 2019

I'm still a bit of lost about the download button rendering logic. Would it be better if useArboristUI === false then it falls back to check Fence authz?

@vpsx
Copy link
Contributor Author

vpsx commented Oct 25, 2019

@mfshao See PR description. Doesn't make sense to use useArboristUI as a flag for this case, because a commons can be using centralized auth but not be using the auth-based UI.

Copy link
Collaborator

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

I think it looks good to me. Just a small suggestion

src/CoreMetadata/CoreMetadataHeader.jsx Show resolved Hide resolved
@vpsx vpsx force-pushed the fix/permissions branch 3 times, most recently from 066bf53 to 540c6ae Compare November 1, 2019 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants