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.

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

reduce technical debt #295

Closed
wants to merge 6 commits into from
Closed
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
3 changes: 3 additions & 0 deletions app/aem/actions.checks/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ aem {

dependencies {
implementation(project(":app:aem:api"))

compileOnly("org.projectlombok:lombok:1.18.8")
annotationProcessor("org.projectlombok:lombok:1.18.8")
}

tasks {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@

package com.cognifide.apm.checks.actions;

public class ActionGroup {
import lombok.AccessLevel;
import lombok.NoArgsConstructor;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class ActionGroup {

public static final String CHECKS = "checks";
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ private ActionResult process(final Context context, boolean execute) {
}

private boolean checkMembers(final Context context, final ActionResult actionResult, final Group group,
final List<String> errors) {
final List<String> errors) {
boolean checkFailed = false;
for (String authorizableId : authorizableIds) {
try {
Expand All @@ -96,16 +96,11 @@ private boolean checkMembers(final Context context, final ActionResult actionRes
}

private Group tryGetGroup(final Context context, final ActionResult actionResult) {
Group group;

Group group = null;
try {
group = context.getAuthorizableManager().getGroup(groupId);
} catch (RepositoryException e) {
actionResult.logError(MessagingUtils.createMessage(e));
return null;
} catch (ActionExecutionException e) {
} catch (RepositoryException | ActionExecutionException e) {
actionResult.logError(MessagingUtils.createMessage(e));
return null;
}
return group;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,14 @@
import com.cognifide.apm.checks.utils.ActionUtils;
import com.cognifide.apm.checks.utils.MessagingUtils;
import com.day.cq.security.util.CqActions;
import com.google.common.base.Function;
import com.google.common.collect.Lists;
import java.security.Principal;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import javax.jcr.Node;
import javax.jcr.NodeIterator;
import javax.jcr.PathNotFoundException;
Expand All @@ -58,7 +57,7 @@ public class CheckPermissions implements Action {
private final String authorizableId;

public CheckPermissions(final String authorizableId, final String path, final String glob,
final List<String> permissions, boolean isAllow) {
final List<String> permissions, boolean isAllow) {
this.authorizableId = authorizableId;
this.path = path;
this.glob = glob;
Expand Down Expand Up @@ -109,8 +108,8 @@ private ActionResult process(final Context context, boolean execute) {
}

private void checkPermissionsForGlob(Session session, final boolean execute, final ActionResult actionResult,
final Authorizable authorizable, final Set<Principal> authorizablesToCheck,
final CqActions actions, final List<String> privilegesToCheck)
final Authorizable authorizable, final Set<Principal> authorizablesToCheck,
final CqActions actions, final List<String> privilegesToCheck)
throws RepositoryException, LoginException {
final List<String> subpaths = getAllSubpaths(session, path);
Pattern pattern = Pattern.compile(path + StringUtils.replace(glob, "*", ".*"));
Expand Down Expand Up @@ -139,15 +138,15 @@ private void checkPermissionsForGlob(Session session, final boolean execute, fin
}

private boolean checkPermissionsForPath(final Set<Principal> authorizablesToCheck,
final CqActions actions, final List<String> privilegesToCheck, String subpath)
final CqActions actions, final List<String> privilegesToCheck, String subpath)
throws RepositoryException {
Collection<String> allowedActions = actions.getAllowedActions(subpath, authorizablesToCheck);
final boolean containsAll = allowedActions.containsAll(privilegesToCheck);
return (!containsAll && isAllow) || (containsAll && !isAllow);
}

private void logFailure(boolean execute, ActionResult actionResult, final Authorizable authorizable,
String subpath) throws RepositoryException {
String subpath) throws RepositoryException {
actionResult.logError(
"Not all required privileges are set for " + authorizable.getID() + " on " + subpath);
if (execute) {
Expand Down Expand Up @@ -187,14 +186,7 @@ private Set<Principal> getAuthorizablesToCheck(Authorizable authorizable, Contex
}

private List<String> preparePrivilegesToCheck() throws RepositoryException {
return Lists.transform(permissions, new toLowerCase());
return permissions.stream().map(String::toLowerCase).collect(Collectors.toList());
}

private static class toLowerCase implements Function<String, String> {

@Override
public String apply(String input) {
return input.toLowerCase();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
Expand All @@ -24,56 +24,56 @@
import java.util.Iterator;
import java.util.List;
import javax.jcr.RepositoryException;
import lombok.AccessLevel;
import lombok.NoArgsConstructor;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class ActionUtils {

public static final String ASSERTION_FAILED_MSG = "Assertion failed";
public static final String ASSERTION_FAILED_MSG = "Assertion failed";

private ActionUtils() {
}
/**
* Adding group to another group may result in cyclic relation. Let current group be the group where we
* want to add current authorizable to. If current authorizable contains group such that current group
* belongs to, then we prevent such operation.
*
* @param currentGroup The group where we want to add current authorizable
* @param groupToBeAdded Authorizable we want to add
* @throws ActionExecutionException Throw exception, if adding operation results in cyclic relation
*/
public static void checkCyclicRelations(Group currentGroup, Group groupToBeAdded)
throws ActionExecutionException {
try {
if (groupToBeAdded.getID().equals(currentGroup.getID())) {
throw new ActionExecutionException(MessagingUtils.addingGroupToItself(currentGroup.getID()));
}
Iterator<Group> parents = currentGroup.memberOf();
while (parents.hasNext()) {
Group currentParent = parents.next();
// Is added group among my parents?
if (currentParent.getID().equals(groupToBeAdded.getID())) {
throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
currentGroup.getID(), groupToBeAdded.getID()));
}
// ... and are its children among my parents?
for (Iterator<Authorizable> children = groupToBeAdded.getMembers(); children.hasNext(); ) {
Authorizable currentChild = children.next();
if (currentParent.getID().equals(currentChild.getID())) {
throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
currentChild.getID(), groupToBeAdded.getID()));
}
}
}
} catch (RepositoryException e) {
throw new ActionExecutionException(MessagingUtils.createMessage(e));
}
}

/**
* Adding group to another group may result in cyclic relation. Let current group be the group where we
* want to add current authorizable to. If current authorizable contains group such that current group
* belongs to, then we prevent such operation.
*
* @param currentGroup The group where we want to add current authorizable
* @param groupToBeAdded Authorizable we want to add
* @throws ActionExecutionException Throw exception, if adding operation results in cyclic relation
*/
public static void checkCyclicRelations(Group currentGroup, Group groupToBeAdded)
throws ActionExecutionException {
try {
if (groupToBeAdded.getID().equals(currentGroup.getID())) {
throw new ActionExecutionException(MessagingUtils.addingGroupToItself(currentGroup.getID()));
}
Iterator<Group> parents = currentGroup.memberOf();
while (parents.hasNext()) {
Group currentParent = parents.next();
// Is added group among my parents?
if (currentParent.getID().equals(groupToBeAdded.getID())) {
throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
currentGroup.getID(), groupToBeAdded.getID()));
}
// ... and are its children among my parents?
for (Iterator<Authorizable> children = groupToBeAdded.getMembers(); children.hasNext(); ) {
Authorizable currentChild = children.next();
if (currentParent.getID().equals(currentChild.getID())) {
throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
currentChild.getID(), groupToBeAdded.getID()));
}
}
}
} catch (RepositoryException e) {
throw new ActionExecutionException(MessagingUtils.createMessage(e));
}
}

public static void logErrors(List<String> errors, ActionResult actionResult) {
for (String error : errors) {
actionResult.logError(error);
}
}
public static void logErrors(List<String> errors, ActionResult actionResult) {
for (String error : errors) {
actionResult.logError(error);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@
*/
package com.cognifide.apm.checks.utils;

import lombok.AccessLevel;
import lombok.NoArgsConstructor;
import org.apache.commons.lang.StringUtils;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class MessagingUtils {

private MessagingUtils() {
}

public static String createMessage(Exception e) {
return StringUtils.isBlank(e.getMessage()) ? "Internal error: " + e.getClass() : e.getMessage();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@

package com.cognifide.apm.main.actions;

public class ActionGroup {
import lombok.AccessLevel;
import lombok.NoArgsConstructor;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class ActionGroup {

public static final String CORE = "core";
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

package com.cognifide.apm.main.actions;

import lombok.AccessLevel;
import lombok.NoArgsConstructor;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class CommonFlags {

public static final String IF_EXISTS = "IF-EXISTS";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,14 @@
import java.util.ArrayList;
import java.util.List;
import javax.jcr.RepositoryException;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.StringUtils;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@Slf4j
public class AddChildren implements Action {

private static final Logger LOGGER = LoggerFactory.getLogger(AddChildren.class);

private final List<String> authorizableIds;

public AddChildren(final List<String> authorizableIds) {
Expand All @@ -60,12 +58,9 @@ private ActionResult process(Context context, boolean execute) {
try {
group = context.getCurrentGroup();
actionResult.setAuthorizable(group.getID());
LOGGER.info(String.format("Adding authorizables %s to group with id = %s",
StringUtils.join(authorizableIds, ", "), group.getID()));
} catch (ActionExecutionException e) {
actionResult.logError(MessagingUtils.createMessage(e));
return actionResult;
} catch (RepositoryException e) {
log.info("Adding authorizables {} to group with id = {}",
StringUtils.join(authorizableIds, ", "), group.getID());
} catch (ActionExecutionException | RepositoryException e) {
actionResult.logError(MessagingUtils.createMessage(e));
return actionResult;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,13 @@
import java.util.ArrayList;
import java.util.List;
import javax.jcr.RepositoryException;
import lombok.extern.slf4j.Slf4j;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@Slf4j
public class AddParents implements Action {

private static final Logger LOGGER = LoggerFactory.getLogger(AddParents.class);

private final List<String> groupIds;

public AddParents(final List<String> groupIds) {
Expand Down Expand Up @@ -72,8 +70,8 @@ private ActionResult process(final Context context, boolean execute) {
if (authorizable.isGroup()) {
ActionUtils.checkCyclicRelations(group, (Group) authorizable);
}
LOGGER.info(String.format("Adding Authorizable with id = %s to group with id = %s",
authorizable.getID(), group.getID()));
log.info("Adding Authorizable with id = {} to group with id = {}",
authorizable.getID(), group.getID());

if (execute) {
group.addMember(authorizable);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,13 @@
import java.util.List;
import javax.jcr.PathNotFoundException;
import javax.jcr.RepositoryException;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.StringUtils;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@Slf4j
public class Allow implements Action {

private static final Logger LOGGER = LoggerFactory.getLogger(Allow.class);

private final String path;

private final List<String> permissions;
Expand All @@ -49,8 +47,8 @@ public class Allow implements Action {
private final boolean ignoreNonExistingPaths;

public Allow(String path, List<String> permissions,
String glob, List<String> ntNames,
List<String> itemNames, boolean ignoreNonExistingPaths) {
String glob, List<String> ntNames,
List<String> itemNames, boolean ignoreNonExistingPaths) {
this.path = path;
this.permissions = permissions;
this.restrictions = new Restrictions(glob, ntNames, itemNames);
Expand All @@ -75,8 +73,8 @@ private ActionResult process(final Context context, boolean simulate) {
context.getSession().getNode(path);
final PermissionActionHelper permissionActionHelper = new PermissionActionHelper(
context.getValueFactory(), path, permissions, restrictions);
LOGGER.info(String.format("Adding permissions %s for authorizable with id = %s for path = %s %s",
permissions.toString(), context.getCurrentAuthorizable().getID(), path, restrictions));
log.info("Adding permissions {} for authorizable with id = {} for path = {} {}",
permissions.toString(), context.getCurrentAuthorizable().getID(), path, restrictions);
if (simulate) {
permissionActionHelper.checkPermissions(context.getAccessControlManager());
} else {
Expand Down
Loading