-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
BUGFIX: show workspace owner/title in media usage tab #5182
base: 8.3
Are you sure you want to change the base?
BUGFIX: show workspace owner/title in media usage tab #5182
Conversation
- configurable privilege roles Issue: neos#5181
621b27e
to
0e15553
Compare
# By default, disable showing the workspace owner/title in media browser usage tab | ||
showWorkspaceOwnerOrName: | ||
enable: false | ||
privilegedRoles: ['Neos.Neos:Administrator'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you didn't introduce an actual privilege instead of defining a privilege via the settings?
You could introduce a new privilege and integrators can add it to whichever role they want.
Could also be enabled by default for admins IMO.
This way you only need to check if the new privilege is granted and simplify the code a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, haven't thought about that one.
I've implemented the privilege but im stuck trying to check the role without looping over them.
Would be nice to have something like Account::hasRole()
including parent roles.
Or should i just loop over the user (parent) roles and match them with the new privilege role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either use $this->securityContext->hasRole($roleIdentifier)
or $this->privilegeManager->isPrivilegeTargetGranted($privilegeTarget);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks worked like a charm!
- new role `Neos.Media.Browser:WorkspaceName` - add new role to `Neos.Neos:Administrator` as parent role - enable showing the workspace owner by default - adjust UsageController.php to check if current user role matches the new role Issue: neos#5181
- remove unused variable `$policyService` and import in UsageController.php and fix formatting - correct commentary in Settings.yaml Issue: neos#5181
Neos.Neos/Configuration/Policy.yaml
Outdated
@@ -325,8 +325,7 @@ roles: | |||
'Neos.Neos:Administrator': | |||
label: Neos Administrator | |||
description: Grants access to all modules and functionalities of the Neos backend. | |||
|
|||
parentRoles: ['Neos.Neos:Editor'] | |||
parentRoles: ['Neos.Neos:Editor', 'Neos.Media.Browser:WorkspaceName'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to assign the privilege in the Media.Browser package to not add a dependency here.
This is already done there for several other privileges like this example:
'Neos.Neos:Administrator':
privileges:
-
privilegeTarget: 'Neos.Media.Browser:ManageAssetCollections'
permission: GRANT
@@ -27,6 +27,9 @@ Neos: | |||
# By default, enable the option to create redirects for replaced asset resources | |||
createAssetRedirectsOption: | |||
enable: true | |||
# By default, enable showing the workspace owner/title in media browser usage tab | |||
showWorkspaceOwnerOrName: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we don't need this switch. If someone doesn't want the feature they can disable or remove the privilege.
@@ -29,6 +29,9 @@ privilegeTargets: | |||
matcher: 'method(Neos\Media\Browser\Controller\(Asset|Image)Controller->(replaceAssetResource|updateAssetResource)Action())' | |||
|
|||
roles: | |||
'Neos.Media.Browser:WorkspaceName': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just assign the privilege to the admin, we don't need this role IMO. If someone needs it, they can create their own.
…ame` instead of the role - introduce new privilege target `Neos.Media.Browser:WorkspaceName`, grant privilege to `Neos.Neos:Administrator` and remove obsolet abstract role `Neos.Media.Browser:WorkspaceName` - remove unnecessary parent role from `Neos.Neos:Administrator` - remove config in Settings.yaml - adjust UsageController.php to work with new privilege target Issue: neos#5181
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are getting there, thx for your work :)
I have an idea: you don't use the isPrivilegedRole
variable but instead add a field label
to the inaccessibleRelation
entries.
There you use a new method getLabelForInaccessibleWorkspace
which is checked by your WorkspaceName
privilege.
If $isPrivilegedRole
is not given, then you leave the label
for the relation empty for now (maybe we can make this nicer in the future and also add the translated part in there.
This way you can just render the label in the template the same way in all 3 cases.
Wdyt? Hope I don't discourage you with my feedback, but things like this are sometimes a bit hard to get right, even though they look simple.
@@ -28,6 +28,10 @@ privilegeTargets: | |||
label: Allowed to replace asset resources | |||
matcher: 'method(Neos\Media\Browser\Controller\(Asset|Image)Controller->(replaceAssetResource|updateAssetResource)Action())' | |||
|
|||
'Neos.Media.Browser:WorkspaceName': | |||
label: Allowed to see workspace owner | |||
matcher: 'method(Neos\Media\Browser\Controller\UsageController->relatedNodesAction())' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think you now blocked access to the relatedNodes for everyone except admins and the check inside the method doesn't do anything as it's the same what the privilege does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried it with a normal Editor and he can access the media usage tab, but doesn't see the workspace titles (as expected). Do you maybe have another solution for the privilege?
- UsageController.php: introduce new method to return `label` for workspace title based on privilege target and workspace type - RelatedNodes.html: use new `label` for rendering Issue: neos#5181
Any updates here?🤔 |
Issue: #5181
Possibility to enable showing all workspace titles/owners to user with certain roles in the media usage tab.
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions