Reduce the number of nested if statements

Refactor `HeadersBeanDefinitionParser` and `AclImpl` to reduce the
number of nested if statements. A few extracted methods are now used
to hopefully improve readability.

Issue gh-8945

acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java

@@ -31,6 +31,7 @@ import org.springframework.security.acls.model.PermissionGrantingStrategy;
 import org.springframework.security.acls.model.Sid;
 import org.springframework.security.acls.model.UnloadedSidException;
 import org.springframework.util.Assert;
+import org.springframework.util.ObjectUtils;
 
 /**
  * Base implementation of <code>Acl</code>.
@@ -285,36 +286,22 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
 
 	@Override
 	public boolean equals(Object obj) {
-		if (obj instanceof AclImpl) {
-			AclImpl rhs = (AclImpl) obj;
-			if (this.aces.equals(rhs.aces)) {
-				if ((this.parentAcl == null && rhs.parentAcl == null)
-						|| (this.parentAcl != null && this.parentAcl.equals(rhs.parentAcl))) {
-					if ((this.objectIdentity == null && rhs.objectIdentity == null)
-							|| (this.objectIdentity != null && this.objectIdentity.equals(rhs.objectIdentity))) {
-						if ((this.id == null && rhs.id == null) || (this.id != null && this.id.equals(rhs.id))) {
-							if ((this.owner == null && rhs.owner == null)
-									|| (this.owner != null && this.owner.equals(rhs.owner))) {
-								if (this.entriesInheriting == rhs.entriesInheriting) {
-									if ((this.loadedSids == null && rhs.loadedSids == null)) {
-										return true;
-									}
-									if (this.loadedSids != null && (this.loadedSids.size() == rhs.loadedSids.size())) {
-										for (int i = 0; i < this.loadedSids.size(); i++) {
-											if (!this.loadedSids.get(i).equals(rhs.loadedSids.get(i))) {
-												return false;
-											}
-										}
-										return true;
-									}
-								}
-							}
-						}
-					}
-				}
-			}
+		if (obj == this) {
+			return true;
 		}
-		return false;
+		if (obj == null || !(obj instanceof AclImpl)) {
+			return false;
+		}
+		AclImpl other = (AclImpl) obj;
+		boolean result = true;
+		result = result && this.aces.equals(other.aces);
+		result = result && ObjectUtils.nullSafeEquals(this.parentAcl, other.parentAcl);
+		result = result && ObjectUtils.nullSafeEquals(this.objectIdentity, other.objectIdentity);
+		result = result && ObjectUtils.nullSafeEquals(this.id, other.id);
+		result = result && ObjectUtils.nullSafeEquals(this.owner, other.owner);
+		result = result && this.entriesInheriting == other.entriesInheriting;
+		result = result && ObjectUtils.nullSafeEquals(this.loadedSids, other.loadedSids);
+		return result;
 	}
 
 	@Override

config/src/main/java/org/springframework/security/config/http/HeadersBeanDefinitionParser.java

@@ -426,78 +426,81 @@ public class HeadersBeanDefinitionParser implements BeanDefinitionParser {
 
 	private void parseFrameOptionsElement(boolean addIfNotPresent, Element element, ParserContext parserContext) {
 		BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(XFrameOptionsHeaderWriter.class);
-
-		Element frameElt = element == null ? null : DomUtils.getChildElementByTagName(element, FRAME_OPTIONS_ELEMENT);
-		if (frameElt != null) {
-			String header = getAttribute(frameElt, ATT_POLICY, null);
-			boolean disabled = "true".equals(getAttribute(frameElt, ATT_DISABLED, "false"));
-
-			if (disabled && header != null) {
-				this.attrNotAllowed(parserContext, ATT_DISABLED, ATT_POLICY, frameElt);
-			}
-			if (!StringUtils.hasText(header)) {
-				header = "DENY";
+		Element frameElement = (element != null) ? DomUtils.getChildElementByTagName(element, FRAME_OPTIONS_ELEMENT)
+				: null;
+		if (frameElement == null) {
+			if (addIfNotPresent) {
+				this.headerWriters.add(builder.getBeanDefinition());
 			}
+			return;
+		}
+		String header = getAttribute(frameElement, ATT_POLICY, null);
+		boolean disabled = "true".equals(getAttribute(frameElement, ATT_DISABLED, "false"));
+		if (disabled && header != null) {
+			this.attrNotAllowed(parserContext, ATT_DISABLED, ATT_POLICY, frameElement);
+		}
+		header = StringUtils.hasText(header) ? header : "DENY";
+		if (ALLOW_FROM.equals(header)) {
+			parseAllowFromFrameOptionsElement(parserContext, builder, frameElement);
+		}
+		else {
+			builder.addConstructorArgValue(header);
+		}
+		if (!disabled) {
+			this.headerWriters.add(builder.getBeanDefinition());
+		}
+	}
 
-			if (ALLOW_FROM.equals(header)) {
-				String strategyRef = getAttribute(frameElt, ATT_REF, null);
-				String strategy = getAttribute(frameElt, ATT_STRATEGY, null);
-
-				if (StringUtils.hasText(strategy) && StringUtils.hasText(strategyRef)) {
-					parserContext.getReaderContext().error("Only one of 'strategy' or 'strategy-ref' can be set.",
-							frameElt);
-				}
-				else if (strategyRef != null) {
-					builder.addConstructorArgReference(strategyRef);
-				}
-				else if (strategy != null) {
-					String value = getAttribute(frameElt, ATT_VALUE, null);
-					if (!StringUtils.hasText(value)) {
-						parserContext.getReaderContext().error("Strategy requires a 'value' to be set.", frameElt);
-					}
-					// static, whitelist, regexp
-					if ("static".equals(strategy)) {
-						try {
-							builder.addConstructorArgValue(new StaticAllowFromStrategy(new URI(value)));
-						}
-						catch (URISyntaxException e) {
-							parserContext.getReaderContext().error("'value' attribute doesn't represent a valid URI.",
-									frameElt, e);
-						}
-					}
-					else {
-						BeanDefinitionBuilder allowFromStrategy;
-						if ("whitelist".equals(strategy)) {
-							allowFromStrategy = BeanDefinitionBuilder
-									.rootBeanDefinition(WhiteListedAllowFromStrategy.class);
-							allowFromStrategy.addConstructorArgValue(StringUtils.commaDelimitedListToSet(value));
-						}
-						else {
-							allowFromStrategy = BeanDefinitionBuilder.rootBeanDefinition(RegExpAllowFromStrategy.class);
-							allowFromStrategy.addConstructorArgValue(value);
-						}
-						String fromParameter = getAttribute(frameElt, ATT_FROM_PARAMETER, "from");
-						allowFromStrategy.addPropertyValue("allowFromParameterName", fromParameter);
-						builder.addConstructorArgValue(allowFromStrategy.getBeanDefinition());
-					}
-				}
-				else {
-					parserContext.getReaderContext().error("One of 'strategy' and 'strategy-ref' must be set.",
-							frameElt);
-				}
-			}
-			else {
-				builder.addConstructorArgValue(header);
+	private void parseAllowFromFrameOptionsElement(ParserContext parserContext, BeanDefinitionBuilder builder,
+			Element frameElement) {
+		String strategyRef = getAttribute(frameElement, ATT_REF, null);
+		String strategy = getAttribute(frameElement, ATT_STRATEGY, null);
+		if (StringUtils.hasText(strategy) && StringUtils.hasText(strategyRef)) {
+			parserContext.getReaderContext().error("Only one of 'strategy' or 'strategy-ref' can be set.",
+					frameElement);
+			return;
+		}
+		if (strategyRef != null) {
+			builder.addConstructorArgReference(strategyRef);
+			return;
+		}
+		if (strategy == null) {
+			parserContext.getReaderContext().error("One of 'strategy' and 'strategy-ref' must be set.", frameElement);
+			return;
+		}
+		String value = getAttribute(frameElement, ATT_VALUE, null);
+		if (!StringUtils.hasText(value)) {
+			parserContext.getReaderContext().error("Strategy requires a 'value' to be set.", frameElement);
+			return;
+		}
+		// static, whitelist, regexp
+		if ("static".equals(strategy)) {
+			try {
+				builder.addConstructorArgValue(new StaticAllowFromStrategy(new URI(value)));
 			}
-
-			if (disabled) {
-				return;
+			catch (URISyntaxException e) {
+				parserContext.getReaderContext().error("'value' attribute doesn't represent a valid URI.", frameElement,
+						e);
 			}
+			return;
 		}
+		BeanDefinitionBuilder allowFromStrategy = getAllowFromStrategy(strategy, value);
+		String fromParameter = getAttribute(frameElement, ATT_FROM_PARAMETER, "from");
+		allowFromStrategy.addPropertyValue("allowFromParameterName", fromParameter);
+		builder.addConstructorArgValue(allowFromStrategy.getBeanDefinition());
+	}
 
-		if (addIfNotPresent || frameElt != null) {
-			this.headerWriters.add(builder.getBeanDefinition());
+	private BeanDefinitionBuilder getAllowFromStrategy(String strategy, String value) {
+		if ("whitelist".equals(strategy)) {
+			BeanDefinitionBuilder allowFromStrategy = BeanDefinitionBuilder
+					.rootBeanDefinition(WhiteListedAllowFromStrategy.class);
+			allowFromStrategy.addConstructorArgValue(StringUtils.commaDelimitedListToSet(value));
+			return allowFromStrategy;
 		}
+		BeanDefinitionBuilder allowFromStrategy;
+		allowFromStrategy = BeanDefinitionBuilder.rootBeanDefinition(RegExpAllowFromStrategy.class);
+		allowFromStrategy.addConstructorArgValue(value);
+		return allowFromStrategy;
 	}
 
 	private void parseXssElement(boolean addIfNotPresent, Element element, ParserContext parserContext) {

etc/checkstyle/checkstyle-suppressions.xml

@@ -3,7 +3,6 @@
 		"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
 		"https://checkstyle.org/dtds/suppressions_1_2.dtd">
 <suppressions>
-	<suppress files=".*" checks="NestedIfDepth" />
 	<suppress files=".*" checks="NewlineAtEndOfFile" />
 	<suppress files=".*" checks="NonEmptyAtclauseDescription" />
 	<suppress files=".*" checks="NoWhitespaceBefore" />