Skip to content

Commit 218480f

Browse files
philwebbrwinch
philwebb
authored and
rwinch
committed
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
1 parent 0b4b22f commit 218480f

3 files changed

Lines changed: 84 additions & 95 deletions

File tree

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

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.springframework.security.acls.model.Sid;
3232
import org.springframework.security.acls.model.UnloadedSidException;
3333
import org.springframework.util.Assert;
34+
import org.springframework.util.ObjectUtils;
3435

3536
/**
3637
* Base implementation of <code>Acl</code>.
@@ -285,36 +286,22 @@ public void updateAuditing(int aceIndex, boolean auditSuccess, boolean auditFail
285286

286287
@Override
287288
public boolean equals(Object obj) {
288-
if (obj instanceof AclImpl) {
289-
AclImpl rhs = (AclImpl) obj;
290-
if (this.aces.equals(rhs.aces)) {
291-
if ((this.parentAcl == null && rhs.parentAcl == null)
292-
|| (this.parentAcl != null && this.parentAcl.equals(rhs.parentAcl))) {
293-
if ((this.objectIdentity == null && rhs.objectIdentity == null)
294-
|| (this.objectIdentity != null && this.objectIdentity.equals(rhs.objectIdentity))) {
295-
if ((this.id == null && rhs.id == null) || (this.id != null && this.id.equals(rhs.id))) {
296-
if ((this.owner == null && rhs.owner == null)
297-
|| (this.owner != null && this.owner.equals(rhs.owner))) {
298-
if (this.entriesInheriting == rhs.entriesInheriting) {
299-
if ((this.loadedSids == null && rhs.loadedSids == null)) {
300-
return true;
301-
}
302-
if (this.loadedSids != null && (this.loadedSids.size() == rhs.loadedSids.size())) {
303-
for (int i = 0; i < this.loadedSids.size(); i++) {
304-
if (!this.loadedSids.get(i).equals(rhs.loadedSids.get(i))) {
305-
return false;
306-
}
307-
}
308-
return true;
309-
}
310-
}
311-
}
312-
}
313-
}
314-
}
315-
}
289+
if (obj == this) {
290+
return true;
316291
}
317-
return false;
292+
if (obj == null || !(obj instanceof AclImpl)) {
293+
return false;
294+
}
295+
AclImpl other = (AclImpl) obj;
296+
boolean result = true;
297+
result = result && this.aces.equals(other.aces);
298+
result = result && ObjectUtils.nullSafeEquals(this.parentAcl, other.parentAcl);
299+
result = result && ObjectUtils.nullSafeEquals(this.objectIdentity, other.objectIdentity);
300+
result = result && ObjectUtils.nullSafeEquals(this.id, other.id);
301+
result = result && ObjectUtils.nullSafeEquals(this.owner, other.owner);
302+
result = result && this.entriesInheriting == other.entriesInheriting;
303+
result = result && ObjectUtils.nullSafeEquals(this.loadedSids, other.loadedSids);
304+
return result;
318305
}
319306

320307
@Override

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

Lines changed: 68 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -426,78 +426,81 @@ private void addContentTypeOptions() {
426426

427427
private void parseFrameOptionsElement(boolean addIfNotPresent, Element element, ParserContext parserContext) {
428428
BeanDefinitionBuilder builder = BeanDefinitionBuilder.genericBeanDefinition(XFrameOptionsHeaderWriter.class);
429-
430-
Element frameElt = element == null ? null : DomUtils.getChildElementByTagName(element, FRAME_OPTIONS_ELEMENT);
431-
if (frameElt != null) {
432-
String header = getAttribute(frameElt, ATT_POLICY, null);
433-
boolean disabled = "true".equals(getAttribute(frameElt, ATT_DISABLED, "false"));
434-
435-
if (disabled && header != null) {
436-
this.attrNotAllowed(parserContext, ATT_DISABLED, ATT_POLICY, frameElt);
437-
}
438-
if (!StringUtils.hasText(header)) {
439-
header = "DENY";
429+
Element frameElement = (element != null) ? DomUtils.getChildElementByTagName(element, FRAME_OPTIONS_ELEMENT)
430+
: null;
431+
if (frameElement == null) {
432+
if (addIfNotPresent) {
433+
this.headerWriters.add(builder.getBeanDefinition());
440434
}
435+
return;
436+
}
437+
String header = getAttribute(frameElement, ATT_POLICY, null);
438+
boolean disabled = "true".equals(getAttribute(frameElement, ATT_DISABLED, "false"));
439+
if (disabled && header != null) {
440+
this.attrNotAllowed(parserContext, ATT_DISABLED, ATT_POLICY, frameElement);
441+
}
442+
header = StringUtils.hasText(header) ? header : "DENY";
443+
if (ALLOW_FROM.equals(header)) {
444+
parseAllowFromFrameOptionsElement(parserContext, builder, frameElement);
445+
}
446+
else {
447+
builder.addConstructorArgValue(header);
448+
}
449+
if (!disabled) {
450+
this.headerWriters.add(builder.getBeanDefinition());
451+
}
452+
}
441453

442-
if (ALLOW_FROM.equals(header)) {
443-
String strategyRef = getAttribute(frameElt, ATT_REF, null);
444-
String strategy = getAttribute(frameElt, ATT_STRATEGY, null);
445-
446-
if (StringUtils.hasText(strategy) && StringUtils.hasText(strategyRef)) {
447-
parserContext.getReaderContext().error("Only one of 'strategy' or 'strategy-ref' can be set.",
448-
frameElt);
449-
}
450-
else if (strategyRef != null) {
451-
builder.addConstructorArgReference(strategyRef);
452-
}
453-
else if (strategy != null) {
454-
String value = getAttribute(frameElt, ATT_VALUE, null);
455-
if (!StringUtils.hasText(value)) {
456-
parserContext.getReaderContext().error("Strategy requires a 'value' to be set.", frameElt);
457-
}
458-
// static, whitelist, regexp
459-
if ("static".equals(strategy)) {
460-
try {
461-
builder.addConstructorArgValue(new StaticAllowFromStrategy(new URI(value)));
462-
}
463-
catch (URISyntaxException e) {
464-
parserContext.getReaderContext().error("'value' attribute doesn't represent a valid URI.",
465-
frameElt, e);
466-
}
467-
}
468-
else {
469-
BeanDefinitionBuilder allowFromStrategy;
470-
if ("whitelist".equals(strategy)) {
471-
allowFromStrategy = BeanDefinitionBuilder
472-
.rootBeanDefinition(WhiteListedAllowFromStrategy.class);
473-
allowFromStrategy.addConstructorArgValue(StringUtils.commaDelimitedListToSet(value));
474-
}
475-
else {
476-
allowFromStrategy = BeanDefinitionBuilder.rootBeanDefinition(RegExpAllowFromStrategy.class);
477-
allowFromStrategy.addConstructorArgValue(value);
478-
}
479-
String fromParameter = getAttribute(frameElt, ATT_FROM_PARAMETER, "from");
480-
allowFromStrategy.addPropertyValue("allowFromParameterName", fromParameter);
481-
builder.addConstructorArgValue(allowFromStrategy.getBeanDefinition());
482-
}
483-
}
484-
else {
485-
parserContext.getReaderContext().error("One of 'strategy' and 'strategy-ref' must be set.",
486-
frameElt);
487-
}
488-
}
489-
else {
490-
builder.addConstructorArgValue(header);
454+
private void parseAllowFromFrameOptionsElement(ParserContext parserContext, BeanDefinitionBuilder builder,
455+
Element frameElement) {
456+
String strategyRef = getAttribute(frameElement, ATT_REF, null);
457+
String strategy = getAttribute(frameElement, ATT_STRATEGY, null);
458+
if (StringUtils.hasText(strategy) && StringUtils.hasText(strategyRef)) {
459+
parserContext.getReaderContext().error("Only one of 'strategy' or 'strategy-ref' can be set.",
460+
frameElement);
461+
return;
462+
}
463+
if (strategyRef != null) {
464+
builder.addConstructorArgReference(strategyRef);
465+
return;
466+
}
467+
if (strategy == null) {
468+
parserContext.getReaderContext().error("One of 'strategy' and 'strategy-ref' must be set.", frameElement);
469+
return;
470+
}
471+
String value = getAttribute(frameElement, ATT_VALUE, null);
472+
if (!StringUtils.hasText(value)) {
473+
parserContext.getReaderContext().error("Strategy requires a 'value' to be set.", frameElement);
474+
return;
475+
}
476+
// static, whitelist, regexp
477+
if ("static".equals(strategy)) {
478+
try {
479+
builder.addConstructorArgValue(new StaticAllowFromStrategy(new URI(value)));
491480
}
492-
493-
if (disabled) {
494-
return;
481+
catch (URISyntaxException e) {
482+
parserContext.getReaderContext().error("'value' attribute doesn't represent a valid URI.", frameElement,
483+
e);
495484
}
485+
return;
496486
}
487+
BeanDefinitionBuilder allowFromStrategy = getAllowFromStrategy(strategy, value);
488+
String fromParameter = getAttribute(frameElement, ATT_FROM_PARAMETER, "from");
489+
allowFromStrategy.addPropertyValue("allowFromParameterName", fromParameter);
490+
builder.addConstructorArgValue(allowFromStrategy.getBeanDefinition());
491+
}
497492

498-
if (addIfNotPresent || frameElt != null) {
499-
this.headerWriters.add(builder.getBeanDefinition());
493+
private BeanDefinitionBuilder getAllowFromStrategy(String strategy, String value) {
494+
if ("whitelist".equals(strategy)) {
495+
BeanDefinitionBuilder allowFromStrategy = BeanDefinitionBuilder
496+
.rootBeanDefinition(WhiteListedAllowFromStrategy.class);
497+
allowFromStrategy.addConstructorArgValue(StringUtils.commaDelimitedListToSet(value));
498+
return allowFromStrategy;
500499
}
500+
BeanDefinitionBuilder allowFromStrategy;
501+
allowFromStrategy = BeanDefinitionBuilder.rootBeanDefinition(RegExpAllowFromStrategy.class);
502+
allowFromStrategy.addConstructorArgValue(value);
503+
return allowFromStrategy;
501504
}
502505

503506
private void parseXssElement(boolean addIfNotPresent, Element element, ParserContext parserContext) {

‎etc/checkstyle/checkstyle-suppressions.xml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
44
"https://checkstyle.org/dtds/suppressions_1_2.dtd">
55
<suppressions>
6-
<suppress files=".*" checks="NestedIfDepth" />
76
<suppress files=".*" checks="NewlineAtEndOfFile" />
87
<suppress files=".*" checks="NonEmptyAtclauseDescription" />
98
<suppress files=".*" checks="NoWhitespaceBefore" />

0 commit comments

Comments
 (0)