Skip to content
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion Classes/Aspects/AugmentationAspect.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Neos\Neos\Domain\Service\ContentContext;
use Neos\Neos\Ui\Domain\Service\UserLocaleService;
use Neos\Neos\Ui\Fusion\Helper\NodeInfoHelper;
use Neos\Neos\Ui\Service\NodePolicyService;

/**
* - Serialize all nodes related to the currently rendered document
Expand Down Expand Up @@ -61,6 +62,12 @@ class AugmentationAspect
*/
protected $controllerContext = null;

/**
* @Flow\Inject
* @var NodePolicyService
*/
protected $nodePolicyService;

/**
* @Flow\Before("method(Neos\Neos\Fusion\ContentElementWrappingImplementation->evaluate())")
* @param JoinPointInterface $joinPoint
Expand Down Expand Up @@ -145,7 +152,8 @@ public function editableElementAugmentation(JoinPointInterface $joinPoint)
return $content;
}

if ($this->nodeAuthorizationService->isGrantedToEditNode($node) === false) {
// TODO: this is where we could use a feature flag
if ($this->nodePolicyService->canReadNode($node) === false) {
return $content;
}

Expand Down
17 changes: 17 additions & 0 deletions Classes/Service/NodePolicyService.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public function getNodePolicyInformation(NodeInterface $node): array
'disallowedNodeTypes' => $this->getDisallowedNodeTypes($node),
'canRemove' => $this->canRemoveNode($node),
'canEdit' => $this->canEditNode($node),
'canView' => $this->canReadNode($node),
'disallowedProperties' => $this->getDisallowedProperties($node)
];
}
Expand All @@ -102,6 +103,22 @@ public function isNodeTreePrivilegeGranted(NodeInterface $node): bool
);
}

/**
* @param NodeInterface $node
* @return bool
*/
public function canReadNode(NodeInterface $node): bool
{
if (!isset(self::getUsedPrivilegeClassNames($this->objectManager)[ReadNodePropertyPrivilege::class])) {
Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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 😊

return true;
}

return $this->privilegeManager->isGranted(
ReadNodePropertyPrivilege::class,
new NodePrivilegeSubject($node)
);
}

/**
* @param NodeInterface $node
* @return array
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
],
"require": {
"neos/neos": "^8.3.0",
"neos/neos-ui-compiled": "self.version"
"neos/neos-ui-compiled": "8.4.x-dev"
Copy link
Contributor Author

@JamesAlias JamesAlias Feb 12, 2025

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-compiled version.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

☝️

Copy link
Contributor Author

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

},
"autoload": {
"psr-4": {
Expand Down
44 changes: 26 additions & 18 deletions packages/neos-ui-ckeditor5-bindings/src/ckEditorApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const bootstrap = _editorConfig => {
};

export const createEditor = store => async options => {
const {propertyDomNode, propertyName, editorOptions, globalRegistry, userPreferences, onChange} = options;
const {propertyDomNode, propertyName, editorOptions, globalRegistry, userPreferences, onChange, isReadOnly} = options;
const ckEditorConfig = editorConfig.configRegistry.getCkeditorConfig({
editorOptions,
userPreferences,
Expand All @@ -58,26 +58,34 @@ export const createEditor = store => async options => {
return NeosEditor
.create(propertyDomNode, ckEditorConfig)
.then(editor => {
const debouncedOnChange = debounce(() => onChange(cleanupContentBeforeCommit(editor.getData())), 1500, {maxWait: 5000});
editor.model.document.on('change:data', debouncedOnChange);
editor.ui.focusTracker.on('change:isFocused', event => {
if (!event.source.isFocused) {
// when another editor is focused commit all possible pending changes
debouncedOnChange.flush();
return
}
editor.isReadOnly = isReadOnly;
Copy link
Member

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.

Copy link
Contributor Author

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.


currentEditor = editor;
editorConfig.setCurrentlyEditedPropertyName(propertyName);
handleUserInteractionCallback();
});
// Set placeholder text as Data if editor is empty because readOnly mode hides the placeholder
if (isReadOnly && editor.getData() === '') {
editor.setData(ckEditorConfig.placeholder, {doNotFireChange: true});
} else {
const debouncedOnChange = debounce(() => onChange(cleanupContentBeforeCommit(editor.getData())), 1500, {maxWait: 5000});
editor.model.document.on('change:data', debouncedOnChange);
editor.ui.focusTracker.on('change:isFocused', event => {
if (!event.source.isFocused) {
// when another editor is focused commit all possible pending changes
debouncedOnChange.flush();
return
}

currentEditor = editor;
editorConfig.setCurrentlyEditedPropertyName(propertyName);
handleUserInteractionCallback();
});

editor.keystrokes.set('Ctrl+K', (_, cancel) => {
store.dispatch(actions.UI.ContentCanvas.toggleLinkEditor());
cancel();
});
editor.keystrokes.set('Ctrl+K', (_, cancel) => {
store.dispatch(actions.UI.ContentCanvas.toggleLinkEditor());
cancel();
});

editor.model.document.on('change', () => handleUserInteractionCallback());
}

editor.model.document.on('change', () => handleUserInteractionCallback());
return editor;
}).catch(e => {
if (e instanceof TypeError && e.message.match(/Class constructor .* cannot be invoked without 'new'/)) {
Expand Down
6 changes: 4 additions & 2 deletions packages/neos-ui-guest-frame/src/initializeGuestFrame.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,11 @@ export default ({globalRegistry, store}) => function * initializeGuestFrame() {

const editPreviewMode = $get(['ui', 'editPreviewMode'], state);
const editPreviewModes = globalRegistry.get('frontendConfiguration').get('editPreviewModes');
const isWorkspaceReadOnly = selectors.CR.Workspaces.isWorkspaceReadOnlySelector(state);
const currentEditMode = editPreviewModes[editPreviewMode];
if (!currentEditMode || !currentEditMode.isEditingMode || isWorkspaceReadOnly) {

// ReadOnly workspaces are handled by ckEditor directly
// TODO: this will break other editors, correct?
if (!currentEditMode || !currentEditMode.isEditingMode) {
return;
}

Expand Down
20 changes: 17 additions & 3 deletions packages/neos-ui-guest-frame/src/initializePropertyDomNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {actions} from '@neos-project/neos-ui-redux-store';
import {validateElement} from '@neos-project/neos-ui-validators';

import {getGuestFrameWindow, closestContextPathInGuestFrame} from './dom';
// TODO: this import feels foreign to the rest of neos code, but it feels strange to import the complete selectors object
import {isWorkspaceReadOnlySelector} from '@neos-project/neos-ui-redux-store/src/CR/Workspaces/selectors';

export default ({store, globalRegistry, nodeTypesRegistry, inlineEditorRegistry, nodes}) => propertyDomNode => {
const guestFrameWindow = getGuestFrameWindow();
Expand Down Expand Up @@ -61,6 +63,7 @@ export default ({store, globalRegistry, nodeTypesRegistry, inlineEditorRegistry,
try {
if (!propertyDomNode.dataset.neosInlineEditorIsInitialized) {
const userPreferences = $get('user.preferences', store.getState());
const isReadOnly = isWorkspaceReadOnlySelector(store.getState());

createInlineEditor({
propertyDomNode,
Expand All @@ -70,10 +73,21 @@ export default ({store, globalRegistry, nodeTypesRegistry, inlineEditorRegistry,
editorOptions,
globalRegistry,
userPreferences,
persistChange: change => store.dispatch(
actions.Changes.persistChanges([change])
),
isReadOnly,
persistChange: change => {
if (isReadOnly) {
return;
}

store.dispatch(
actions.Changes.persistChanges([change])
)
},
onChange: value => {
if (isReadOnly) {
return;
}

const validationResult = validateElement(value, $get(['properties', propertyName], nodeType), globalRegistry.get('validators'));
// Update inline validation errors
store.dispatch(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export default class TabPanel extends PureComponent {
return false;
}

return $get(['policy', 'canEdit'], node) && !$contains(item.id, 'policy.disallowedProperties', node);
return $get(['policy', 'canView'], node) && !$contains(item.id, 'policy.disallowedProperties', node);
};

renderTabPanel = groups => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ export default class Inspector extends PureComponent {
return false;
}

return $get(['policy', 'canEdit'], focusedNode) && !$contains(item.id, 'policy.disallowedProperties', focusedNode);
return $get(['policy', 'canView'], focusedNode) && !$contains(item.id, 'policy.disallowedProperties', focusedNode);
};

/**
Expand Down
Loading