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 2 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
3 changes: 3 additions & 0 deletions modules/core/core-api/src/main/java/com/enonic/xp/node/ApplyNodePermissionsParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ private ApplyNodePermissionsParams( Builder builder )

Preconditions.checkArgument( permissions.isEmpty() || ( addPermissions.isEmpty() && removePermissions.isEmpty() ),
"Permissions cannot be set together with addPermissions or removePermissions" );

Preconditions.checkArgument( !permissions.isEmpty() || !addPermissions.isEmpty() || !removePermissions.isEmpty(),
"At least one of permissions, addPermissions or removePermissions must be set" );
}

public static Builder create()
Expand Down
20 changes: 17 additions & 3 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 @@ -2,6 +2,7 @@

import java.util.Collection;
import java.util.Objects;
import java.util.Optional;

import com.enonic.xp.branch.Branches;
import com.enonic.xp.content.ApplyContentPermissionsParams;
Expand All @@ -12,10 +13,12 @@
import com.enonic.xp.context.ContextBuilder;
import com.enonic.xp.node.ApplyNodePermissionsParams;
import com.enonic.xp.node.ApplyNodePermissionsResult;
import com.enonic.xp.node.Node;
import com.enonic.xp.node.NodeCommitEntry;
import com.enonic.xp.node.NodeId;
import com.enonic.xp.node.RoutableNodeVersionId;
import com.enonic.xp.node.RoutableNodeVersionIds;
import com.enonic.xp.security.acl.AccessControlList;


final class ApplyContentPermissionsCommand
Expand All @@ -36,12 +39,23 @@ ApplyContentPermissionsResult execute()
{
final NodeId nodeId = NodeId.from( params.getContentId() );

AccessControlList permissions = params.getPermissions();

if ( params.getPermissions().isEmpty() && params.getAddPermissions().isEmpty() && params.getRemovePermissions().isEmpty() )
{
permissions = Optional.of( nodeService.getById( nodeId ) )
.map( Node::getPermissions )
.orElseThrow( () -> new IllegalArgumentException( "permissions is not set and node not found" ) );
}

final ApplyNodePermissionsParams.Builder applyNodePermissionsBuilder = ApplyNodePermissionsParams.create()
.nodeId( nodeId )
.permissions( params.getPermissions() )
.addPermissions( params.getAddPermissions() ).removePermissions( params.getRemovePermissions() ).scope( params.getScope() )
.permissions( permissions )
.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: 75 additions & 90 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 @@

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,89 @@

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(), params.getPermissions() );

refresh( RefreshMode.ALL );

return results.build();
final ApplyNodePermissionsResult result = results.build();
if ( result.getResults().isEmpty() )
{
throw new NodeNotFoundException( "Node not found: " + params.getNodeId() );

Check warning on line 88 in modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/node/ApplyNodePermissionsCommand.java

View check run for this annotation

Codecov / codecov/patch

modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/node/ApplyNodePermissionsCommand.java#L88 

Added line #L88 was not covered by tests
}
return result;
}

private void doApplyPermissions( final NodeId nodeId, final AccessControlList permissions )
{
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.CHILDREN == params.getScope() && params.getNodeId().equals( nodeId ) )
{
updatedPersistedNode = doApplyOnNode( nodeId, permissions );
doApplyOnChildren( permissions, persistedNode.path() );
}

if ( updatedPersistedNode == null )
else
{
return;
doApplyOnNode( nodeId, permissions );
}

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 ) );
}

private void doApplyOnNode( final NodeId nodeId, final AccessControlList permissions, final Branches branches,
final Map<Branch, NodeVersionMetadata> activeVersionMap, final boolean recursive )
{

results.addResult( nodeId, this.sourceBranch, updatedOriginNode.node() );
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 +167,7 @@

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

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

Expand All @@ -195,7 +180,7 @@

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

return new NodeVersionData( nodeStorageService.get( updatedVersionMetadata.getNodeVersionId(), targetContext ),
updatedVersionMetadata );
}
else
{
Expand All @@ -231,14 +218,10 @@
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,
Expand Down Expand Up @@ -282,8 +265,10 @@
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
Loading