-
-
Notifications
You must be signed in to change notification settings - Fork 146
TASK: Add ability for read-only users to interact with nodes in the preview page and view node data in the inspector #3921
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
base: 8.4
Are you sure you want to change the base?
Conversation
…nd view node data in inspector
** THIS HAS TO BE REVERTED BEFORE MERGE ** There is no tag for this dev branch in neos-ui-compiled. just use 8.4.
f5bf20d to
f913346
Compare
| "require": { | ||
| "neos/neos": "^8.3.0", | ||
| "neos/neos-ui-compiled": "self.version" | ||
| "neos/neos-ui-compiled": "8.4.x-dev" |
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.
!!!Revert this before merging!!!
I had to do this to allow composer to install an existing
neos/neos-ui-compiledversion.
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 could have probably also did this override from your root composer.json.
I don' remember having this issue.
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.
☝️
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.
I will revert this right before merging
skurfuerst
left a comment
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.
I really like this change; I reviewed this in depth.
IMHO, the functional change is a BUGFIX; so no need for a feature flag from my side.
Thanks <3
Sebobo
left a comment
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.
Thx for the change, this is useful!
Please look at my suggestion and solve the todos.
bd02edc to
676d25b
Compare
| debouncedOnChange.flush(); | ||
| return | ||
| } | ||
| editor.isReadOnly = isReadOnly; |
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.
i think i also attempted to use the IsReadOnly flag from ck and that did not work always correctly ... instead in #3842 i made it work by just NOT booting the ckeditor when there is no reason too and think we should do the same here? Not booting the ckeditor and still showing an outline or something is imo much more stable.
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.
I see your point and feel free to implement this. For me personally I don't have the time and as long as it works like intended I will just keep it like this.
Sebobo
left a comment
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.
mhsdesign
left a comment
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.
Okay aside from the comment regarding the ckeditor thing i actually cant get this change to work:
The neos ui doesnt go in restricted editing mode and i can still edit things and get raw errors back
Access denied for method Method: Neos\ContentRepository\Domain\Model\Node::setProperty() Evaluated following 1 privilege target(s): "RestrictEditing": ABSTAIN (0 granted, 0 denied, 1 abstained) Authenticated roles: Neos.Flow:Everybody, Neos.Flow:AuthenticatedUser, Neos.Neos:Administrator, Neos.Neos:Editor, Neos.Neos:AbstractEditor, Neos.ContentRepository:Administrator, Neos.ContentRepository:InternalWorkspaceAccess, Neos.Neos:LivePublisher
this is the configuration i used
privilegeTargets:
'Neos\ContentRepository\Security\Authorization\Privilege\Node\EditNodePrivilege':
'RestrictEditing':
matcher: 'isDescendantNodeOf("/sites/")'
'Neos\ContentRepository\Security\Authorization\Privilege\Node\ReadNodePropertyPrivilege':
'RestrictReading':
matcher: 'isDescendantNodeOf("/sites/")'
roles:
'Neos.Neos:Editor':
privileges:
-
privilegeTarget: 'RestrictReading'
permission: GRANT
| */ | ||
| public function canReadNode(NodeInterface $node): bool | ||
| { | ||
| if (!isset(self::getUsedPrivilegeClassNames($this->objectManager)[ReadNodePropertyPrivilege::class])) { |
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.
The symbol ReadNodePropertyPrivilege was not imported and thus this is just a noop and returns always true
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.
And when i import it i get a an exception
ReadNodePropertyPrivilege only support subjects of type PropertyAwareNodePrivilegeSubject or MethodPrivilegeSubject, but we got a subject of type: NodePrivilegeSubject
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 decided to use the ReadNodePrivilege, which actually exists.
Some Context: The user will not see a node he can not read, so this check is obsolete. We still keep it for AOP shenanigans though.
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.
okay i will have to retest now as we use the ReadNodePrivilege now instead of ReadNodePropertyPrivilege
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.
@mhsdesign I will merge this next week on Tuesday or Wednesday.
If you like, you can test this again.
Thank you for the reviews so far 😊
| "require": { | ||
| "neos/neos": "^8.3.0", | ||
| "neos/neos-ui-compiled": "self.version" | ||
| "neos/neos-ui-compiled": "8.4.x-dev" |
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.
☝️



What I did
Previous Behaviour:
Fixed Behaviour:
How I did it
This should also apply to read-only workspaces.
How to verify it
BEFORE


AFTER