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.

Sign up for GitHub

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

Adjust applyPermissions logic #10919 #10928

Merged
merged 5 commits into from
Feb 25, 2025
Merged

Adjust applyPermissions logic #10919 #10928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf197fd
Adjust applyPermissions logic #10919
vbradnitski Feb 20, 2025
47fda55
Adjust applyPermissions logic #10919
vbradnitski Feb 21, 2025
66b4df9
implement attachments patching #10904
vbradnitski Feb 24, 2025
ef1b8a0
implement attachments patching #10904
vbradnitski Feb 24, 2025
538cde4
implement attachments patching #10904
vbradnitski Feb 25, 2025
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
2 changes: 1 addition & 1 deletion modules/core/core-api/src/main/java/com/enonic/xp/node/ApplyPermissionsScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

public enum ApplyPermissionsScope
{
SINGLE, TREE, CHILDREN
SINGLE, TREE, SUBTREE
}
9 changes: 5 additions & 4 deletions ...content/src/main/java/com/enonic/xp/core/impl/content/ApplyContentPermissionsCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ ApplyContentPermissionsResult execute()
final NodeId nodeId = NodeId.from( params.getContentId() );

final ApplyNodePermissionsParams.Builder applyNodePermissionsBuilder = ApplyNodePermissionsParams.create()
.nodeId( nodeId )
.permissions( params.getPermissions() )
.addPermissions( params.getAddPermissions() ).removePermissions( params.getRemovePermissions() ).scope( params.getScope() )
.nodeId( nodeId ).permissions( params.getPermissions() )
.addPermissions( params.getAddPermissions() )
.removePermissions( params.getRemovePermissions() )
.scope( params.getScope() )
.applyPermissionsListener( params.getListener() )
.addBranches( Branches.from( ContentConstants.BRANCH_MASTER ) );
.addBranches( Branches.from( ContentConstants.BRANCH_DRAFT, ContentConstants.BRANCH_MASTER ) );

final ApplyNodePermissionsResult result = nodeService.applyPermissions( applyNodePermissionsBuilder.build() );

Expand Down
165 changes: 74 additions & 91 deletions ...ore/core-repo/src/main/java/com/enonic/xp/repo/impl/node/ApplyNodePermissionsCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import com.google.common.collect.Streams;

import com.enonic.xp.branch.Branch;
import com.enonic.xp.branch.Branches;
import com.enonic.xp.content.ApplyPermissionsListener;
import com.enonic.xp.context.Context;
import com.enonic.xp.context.ContextAccessor;
import com.enonic.xp.context.ContextBuilder;
import com.enonic.xp.node.ApplyNodePermissionsParams;
Expand All @@ -22,7 +21,9 @@
import com.enonic.xp.node.NodeId;
import com.enonic.xp.node.NodeIds;
import com.enonic.xp.node.NodeNotFoundException;
import com.enonic.xp.node.NodePath;
import com.enonic.xp.node.NodeQuery;
import com.enonic.xp.node.NodeVersionId;
import com.enonic.xp.node.NodeVersionMetadata;
import com.enonic.xp.node.Nodes;
import com.enonic.xp.node.PushNodeEntry;
Expand All @@ -46,17 +47,21 @@ public class ApplyNodePermissionsCommand

private final ApplyNodePermissionsResult.Builder results;

private final Branch sourceBranch;

private final ApplyPermissionsListener listener;

private final Map<NodeVersionId, NodeVersionMetadata> appliedVersions; // old version id -> new version metadata

private final Branches branches;

private ApplyNodePermissionsCommand( final Builder builder )
{
super( builder );
this.params = builder.params;
this.results = ApplyNodePermissionsResult.create();
this.sourceBranch = ContextAccessor.current().getBranch();
listener = params.getListener() != null ? params.getListener() : new EmptyApplyPermissionsListener();
this.appliedVersions = new HashMap<>();
this.listener = params.getListener() != null ? params.getListener() : new EmptyApplyPermissionsListener();
this.branches = params.getBranches().isEmpty() ? Branches.from( ContextAccessor.current().getBranch() ) : params.getBranches();

}

public static Builder create()
Expand All @@ -71,109 +76,84 @@ public static Builder create( AbstractNodeCommand source )

public ApplyNodePermissionsResult execute()
{
if ( params.getBranches().contains( this.sourceBranch ) )
{
throw new IllegalArgumentException( "Branches cannot contain current branch" );
}

final Node persistedNode = doGetById( params.getNodeId() );

if ( persistedNode == null )
{
throw new NodeNotFoundException(
String.format( "Node with id [%s] not found in branch [%s]", params.getNodeId(), sourceBranch ) );
}

AccessControlList permissions = params.getPermissions();

if ( params.getPermissions().isEmpty() && params.getAddPermissions().isEmpty() && params.getRemovePermissions().isEmpty() )
{
permissions = persistedNode.getPermissions();
}

refresh( RefreshMode.SEARCH );

doApplyPermissions( params.getNodeId(), permissions );
doApplyPermissions( params.getNodeId() );

refresh( RefreshMode.ALL );

return results.build();
}

private void doApplyPermissions( final NodeId nodeId, final AccessControlList permissions )
private void doApplyPermissions( final NodeId nodeId )
{
NodeVersionData updatedPersistedNode;
final Node persistedNode =
ContextBuilder.from( ContextAccessor.current() ).branch( branches.first() ).build().callWith( () -> doGetById( nodeId ) );

if ( ApplyPermissionsScope.CHILDREN == params.getScope() && params.getNodeId().equals( nodeId ) )
if ( persistedNode == null )
{
final Node persistedNode = doGetById( nodeId );

if ( persistedNode == null )
{
return;
}

updatedPersistedNode = new NodeVersionData( persistedNode,
nodeStorageService.getVersion( persistedNode.getNodeVersionId(),
InternalContext.from(
ContextAccessor.current() ) ) );
throw new NodeNotFoundException( "Node not found: " + nodeId );
}
else

if ( ApplyPermissionsScope.SUBTREE == params.getScope() && params.getNodeId().equals( nodeId ) )
{
updatedPersistedNode = doApplyOnNode( nodeId, permissions );
doApplyOnChildren( params.getPermissions(), persistedNode.path() );
}

if ( updatedPersistedNode == null )
else
{
return;
doApplyOnNode( nodeId, params.getPermissions() );
}

doApplyOnChildren( permissions, updatedPersistedNode );
}

private NodeVersionData doApplyOnNode( final NodeId nodeId, final AccessControlList permissions )
private void doApplyOnNode( final NodeId nodeId, final AccessControlList permissions )
{
final Map<Branch, NodeVersionMetadata> activeVersionMap = getActiveNodeVersions( nodeId );
final Map<Branch, NodeVersionMetadata> activeVersionMap = getActiveNodeVersions( nodeId, branches );

final NodeVersionData updatedOriginNode = updatePermissionsInBranch( nodeId, null, permissions, this.sourceBranch );
doApplyOnNode( nodeId, permissions, Branches.from( branches.first() ), activeVersionMap, true );

if ( updatedOriginNode == null )
{
results.addResult( nodeId, this.sourceBranch, null );
return null;
}
final Branches otherBranches = activeVersionMap.keySet()
.stream()
.filter( targetBranch -> !targetBranch.equals( branches.first() ) )
.collect( Branches.collecting() );

otherBranches.forEach( targetBranch -> doApplyOnNode( nodeId, permissions, otherBranches, activeVersionMap, false ) );
}

results.addResult( nodeId, this.sourceBranch, updatedOriginNode.node() );
private void doApplyOnNode( final NodeId nodeId, final AccessControlList permissions, final Branches branches,
final Map<Branch, NodeVersionMetadata> activeVersionMap, final boolean recursive )
{

branches.forEach( branch -> {
final NodeVersionData updatedSourceNode = updatePermissionsInBranch( nodeId, appliedVersions.get(
Optional.ofNullable( activeVersionMap.get( branch ) ).map( NodeVersionMetadata::getNodeVersionId ).orElse( null ) ),
permissions, branch );

activeVersionMap.keySet().forEach( targetBranch -> {
if ( targetBranch.equals( this.sourceBranch ) )
if ( updatedSourceNode != null )
{
return;
appliedVersions.put( activeVersionMap.get( branch ).getNodeVersionId(), updatedSourceNode.nodeVersionMetadata() );
}

final boolean isEqualToOrigin =
activeVersionMap.get( targetBranch ).getNodeVersionId().equals( activeVersionMap.get( sourceBranch ).getNodeVersionId() );

final NodeVersionData updatedTargetNode =
updatePermissionsInBranch( nodeId, isEqualToOrigin ? updatedOriginNode.nodeVersionMetadata() : null, permissions,
targetBranch );
;
results.addResult( nodeId, targetBranch, isEqualToOrigin
? updatedOriginNode.node()
: updatedTargetNode != null ? updatedTargetNode.node() : null );
results.addResult( nodeId, branch, updatedSourceNode != null ? updatedSourceNode.node() : null );

if ( recursive )
{
if ( updatedSourceNode != null && updatedSourceNode.node() != null )
Copy link

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium ErrorProne issue: These nested if statements could be combined

The issue identified by the PMD linter is that the nested if statements can be combined into a single conditional expression. This improves the readability of the code by reducing the number of nested blocks and making the logic clearer.

Here’s the original line:

Suggested change
if ( updatedSourceNode != null && updatedSourceNode.node() != null )
if ( updatedSourceNode != null && updatedSourceNode.node() != null )

The suggestion is to combine the conditions into a single line without changing the logic.

Here’s the code suggestion:

Suggested change
if ( updatedSourceNode != null && updatedSourceNode.node() != null )
if ( updatedSourceNode != null && updatedSourceNode.node() != null )

This line is already optimal, but if you want to avoid the nested if statement altogether, you could simplify it by using a single if statement with a method reference or a lambda if appropriate. However, since the request is for a single line change, the original line is already concise and correct.

If you want to directly address the suggestion made by PMD, you could refactor the code to avoid nesting entirely. Here’s a more concise way to express the same logic:

Suggested change
if ( updatedSourceNode != null && updatedSourceNode.node() != null )
if ( updatedSourceNode != null && updatedSourceNode.node() != null ) doApplyOnChildren(permissions, updatedSourceNode.node().path());

This single line change removes the nested structure by directly invoking doApplyOnChildren if both conditions are met.

This comment was generated by an experimental AI tool.

{
doApplyOnChildren( permissions, updatedSourceNode.node().path() );
}
}
} );
return updatedOriginNode;
}

private void doApplyOnChildren( final AccessControlList permissions, final NodeVersionData updatedOriginNode )
private void doApplyOnChildren( final AccessControlList permissions, final NodePath parentPath )
{
final NodeIds childrenIds = NodeIds.from( this.nodeSearchService.query(
NodeQuery.create().size( NodeSearchService.GET_ALL_SIZE_FLAG ).parent( updatedOriginNode.node().path() ).build(),
SingleRepoSearchSource.from( ContextBuilder.from( ContextAccessor.current() ).branch( this.sourceBranch ).build() ) )
.getIds() );
final Context sourceBranchContext = ContextBuilder.from( ContextAccessor.current() ).branch( branches.first() ).build();

final NodeIds childrenIds = NodeIds.from(
this.nodeSearchService.query( NodeQuery.create().size( NodeSearchService.GET_ALL_SIZE_FLAG ).parent( parentPath ).build(),
SingleRepoSearchSource.from( sourceBranchContext ) ).getIds() );

final Nodes children = this.nodeStorageService.get( childrenIds, InternalContext.from( ContextAccessor.current() ) );
final Nodes children = this.nodeStorageService.get( childrenIds, InternalContext.from( sourceBranchContext ) );

for ( Node child : children )
{
Expand All @@ -182,7 +162,7 @@ private void doApplyOnChildren( final AccessControlList permissions, final NodeV

final AccessControlList childPermissions = mergingStrategy.mergePermissions( child.getPermissions(), permissions );

doApplyPermissions( child.id(), childPermissions );
doApplyOnNode( child.id(), childPermissions );
}
}

Expand All @@ -195,7 +175,7 @@ private NodeVersionData updatePermissionsInBranch( final NodeId nodeId, final No

if ( persistedNode == null ||
!NodePermissionsResolver.hasPermission( targetContext.getPrincipalsKeys(), Permission.WRITE_PERMISSIONS,
persistedNode.getPermissions() ) )
persistedNode.getPermissions() ) )
{
listener.notEnoughRights( 1 );
return null;
Expand All @@ -215,7 +195,9 @@ private NodeVersionData updatePermissionsInBranch( final NodeId nodeId, final No
.build() )
.build() ), branch, l -> {
}, InternalContext.from( ContextAccessor.current() ) );
return null;

return new NodeVersionData( nodeStorageService.get( updatedVersionMetadata.getNodeVersionId(), targetContext ),
updatedVersionMetadata );
}
else
{
Expand All @@ -231,24 +213,23 @@ private NodeVersionData updatePermissionsInBranch( final NodeId nodeId, final No
return result;
}

private Map<Branch, NodeVersionMetadata> getActiveNodeVersions( final NodeId nodeId )
private Map<Branch, NodeVersionMetadata> getActiveNodeVersions( final NodeId nodeId, final Branches branches )
{
return params.getBranches() != null ? GetActiveNodeVersionsCommand.create( this )
.nodeId( nodeId )
.branches( Streams.concat( params.getBranches().stream(), Stream.of( this.sourceBranch ) ).collect( Branches.collecting() ) )
.build()
.execute()
.getNodeVersions() : Map.of();

return GetActiveNodeVersionsCommand.create( this ).nodeId( nodeId ).branches( branches ).build().execute().getNodeVersions();
}

private AccessControlList compileNewPermissions( final AccessControlList persistedPermissions, final AccessControlList permissions,
final AccessControlList addPermissions, final AccessControlList removePermissions )
{

if ( !permissions.isEmpty() )
{
return permissions;
}
else if ( addPermissions.isEmpty() && removePermissions.isEmpty() )
{
return AccessControlList.empty();
}

final HashMap<PrincipalKey, AccessControlEntry> newPermissions = new HashMap<>( persistedPermissions.asMap() );

Expand Down Expand Up @@ -282,8 +263,10 @@ private AccessControlList compileNewPermissions( final AccessControlList persist
else
{
newPermissions.put( entryToRemove.getPrincipal(), AccessControlEntry.create()
.principal( entryToRemove.getPrincipal() ).allow( currentACE.allowedPermissions()
.stream().filter( permission -> !entryToRemove.allowedPermissions().contains( permission ) )
.principal( entryToRemove.getPrincipal() )
.allow( currentACE.allowedPermissions()
.stream()
.filter( permission -> !entryToRemove.allowedPermissions().contains( permission ) )
.collect( Collectors.toList() ) )
.build() );
}
Expand Down
9 changes: 0 additions & 9 deletions ...ore/core-security/src/main/java/com/enonic/xp/core/impl/security/SecurityServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.enonic.xp.data.ValueFactory;
import com.enonic.xp.index.IndexPath;
import com.enonic.xp.node.ApplyNodePermissionsParams;
import com.enonic.xp.node.ApplyPermissionsScope;
import com.enonic.xp.node.CreateNodeParams;
import com.enonic.xp.node.DeleteNodeParams;
import com.enonic.xp.node.FindNodesByParentParams;
Expand Down Expand Up @@ -986,10 +985,6 @@ public IdProvider createIdProvider( final CreateIdProviderParams createIdProvide
.permissions( groupsNodePermissions )
.build() );

final ApplyNodePermissionsParams applyPermissions =
ApplyNodePermissionsParams.create().nodeId( rootNode.id() ).scope( ApplyPermissionsScope.SINGLE ).build();
nodeService.applyPermissions( applyPermissions );

return idProviderNode;
} );

Expand Down Expand Up @@ -1058,10 +1053,6 @@ public IdProvider updateIdProvider( final UpdateIdProviderParams updateIdProvide
setNodePermissions( idProviderNode.id(), idProviderNodePermissions );
setNodePermissions( usersNode.id(), usersNodePermissions );
setNodePermissions( groupsNode.id(), groupsNodePermissions );

final ApplyNodePermissionsParams applyPermissions =
ApplyNodePermissionsParams.create().nodeId( idProviderNode.id() ).scope( ApplyPermissionsScope.SINGLE ).build();
nodeService.applyPermissions( applyPermissions );
}

securityAuditLogSupport.updateIdProvider( UpdateIdProviderParams.create( idProviderToUpdate ).build() );
Expand Down
7 changes: 5 additions & 2 deletions ...ore/src/test/java/com/enonic/xp/core/content/ContentServiceImplTest_applyPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,11 @@ public void success()

final ApplyPermissionsListener listener = mock( ApplyPermissionsListener.class );

final ApplyContentPermissionsParams applyParams =
ApplyContentPermissionsParams.create().contentId( content.getId() ).applyContentPermissionsListener( listener ).build();
final ApplyContentPermissionsParams applyParams = ApplyContentPermissionsParams.create()
.contentId( content.getId() )
.applyContentPermissionsListener( listener )
.addPermissions( content.getPermissions() )
.build();

final ApplyContentPermissionsResult result = this.contentService.applyPermissions( applyParams );

Expand Down
Loading