SONARPY-1147 Refactoring before release of SonarPython 3.18

Author: rudy-regazzoni-sonarsource

python-checks/src/main/java/org/sonar/python/checks/cdk/AbstractCdkResourceCheck.java

@@ -19,28 +19,24 @@
  */
 package org.sonar.python.checks.cdk;
 
-import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Collection;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.BiConsumer;
-import java.util.function.Predicate;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.tree.CallExpression;
-import org.sonar.plugins.python.api.tree.Expression;
-import org.sonar.plugins.python.api.tree.Name;
-import org.sonar.plugins.python.api.tree.RegularArgument;
-import org.sonar.plugins.python.api.tree.StringLiteral;
-import org.sonar.plugins.python.api.tree.Token;
 import org.sonar.plugins.python.api.tree.Tree;
-import org.sonar.python.checks.Expressions;
-import org.sonar.python.tree.TreeUtils;
 
+/**
+ * Since most CDK related checks check arguments of method calls or object initializations,
+ * this abstract class can be used to register CallExpression consumers for various fully qualified names.
+ * For this purpose the method {@link #checkFqn(String, BiConsumer)} or {@link #checkFqns(Collection, BiConsumer)}
+ * must be called in the {@link #registerFqnConsumer()} method which has to be implemented.
+ */
 public abstract class AbstractCdkResourceCheck extends PythonSubscriptionCheck {
 
   private final Map<String, BiConsumer<SubscriptionContext, CallExpression>> fqnCallConsumers = new HashMap<>();
@@ -59,150 +55,22 @@ protected void visitNode(SubscriptionContext ctx) {
       .ifPresent(consumer -> consumer.accept(ctx, node));
   }
 
-  // TODO should be abstract when resourceFqn and visitResourceConstructor are dropped
-  protected void registerFqnConsumer() {
-    checkFqn(resourceFqn(), this::visitResourceConstructor);
-  }
-
-  protected void checkFqn(String fqn, BiConsumer<SubscriptionContext, CallExpression> consumer) {
-    fqnCallConsumers.put(fqn, consumer);
-  }
-
-  protected String resourceFqn() {
-    throw new UnsupportedOperationException("Override at least resourceFqn or registerFqnConsumer");
-  }
-
-  protected void visitResourceConstructor(SubscriptionContext ctx, CallExpression resourceConstructor) {
-    throw new UnsupportedOperationException("When using resourceFqn override this method");
-  }
-
-  protected static Optional<ArgumentTrace> getArgument(SubscriptionContext ctx, CallExpression callExpression, String argumentName) {
-    return callExpression.arguments().stream()
-      .map(RegularArgument.class::cast)
-      .filter(regularArgument -> regularArgument.keywordArgument() != null)
-      .filter(regularArgument -> argumentName.equals(regularArgument.keywordArgument().name()))
-      .map(regularArgument -> ArgumentTrace.build(ctx, regularArgument.expression()))
-      .findAny();
-  }
+  protected abstract void registerFqnConsumer();
 
   /**
-   * For compatibility with other classes and branches.
-   * TODO Can be removed at the end of the sprint to reduce complexity.
+   * Register a consumer for a single FQN
    */
-  public static class ArgumentTrace extends ExpressionTrace {
-    private ArgumentTrace(SubscriptionContext ctx, List<Expression> trace) {
-      super(ctx, trace);
-    }
-
-    protected static ArgumentTrace build(SubscriptionContext ctx, Expression expression) {
-      List<Expression> trace = new ArrayList<>();
-      buildTrace(expression, trace);
-      return new ArgumentTrace(ctx, trace);
-    }
-  }
-
-  static class ExpressionTrace {
-
-    private static final String TAIL_MESSAGE = "Propagated setting.";
-
-    private final SubscriptionContext ctx;
-    private final List<Expression> trace;
-
-    private ExpressionTrace(SubscriptionContext ctx, List<Expression> trace) {
-      this.ctx = ctx;
-      this.trace = Collections.unmodifiableList(trace);
-    }
-    protected static ExpressionTrace build(SubscriptionContext ctx, Expression expression) {
-      List<Expression> trace = new ArrayList<>();
-      buildTrace(expression, trace);
-      return new ExpressionTrace(ctx, trace);
-    }
-
-    static void buildTrace(Expression expression, List<Expression> trace) {
-      trace.add(expression);
-      if (expression.is(Tree.Kind.NAME)) {
-        Expression singleAssignedValue = Expressions.singleAssignedValue(((Name) expression));
-        if (singleAssignedValue != null && !trace.contains(singleAssignedValue)) {
-          buildTrace(singleAssignedValue, trace);
-        }
-      }
-    }
-
-    public void addIssue(String primaryMessage) {
-      PreciseIssue issue = ctx.addIssue(trace.get(0).parent(), primaryMessage);
-      trace.stream().skip(1).forEach(expression -> issue.secondary(expression.parent(), TAIL_MESSAGE));
-    }
-
-    public void addIssueIf(Predicate<Expression> predicate, String primaryMessage) {
-      if (hasExpression(predicate)) {
-        addIssue(primaryMessage);
-      }
-    }
-
-    public void addIssueIf(Predicate<Expression> predicate, String primaryMessage, CallExpression call) {
-      if (hasExpression(predicate)) {
-        ctx.addIssue(call.callee(), primaryMessage);
-      }
-    }
-
-    public boolean hasExpression(Predicate<Expression> predicate) {
-      return trace.stream().anyMatch(predicate);
-    }
-
-    public Optional<Expression> getExpression(Predicate<Expression> predicate) {
-      return trace.stream().filter(predicate).findFirst();
-    }
-
-    public List<Expression> trace() {
-      return trace;
-    }
-  }
-
-  protected static Predicate<Expression> isFalse() {
-    return expression -> Optional.ofNullable(expression.firstToken()).map(Token::value).filter("False"::equals).isPresent();
-  }
-
-  protected static Predicate<Expression> isNone() {
-    return expression -> expression.is(Tree.Kind.NONE);
-  }
-
-  protected static Predicate<Expression> isFqn(String fqnValue) {
-    return expression ->  Optional.ofNullable(TreeUtils.fullyQualifiedNameFromExpression(expression))
-      .filter(fqnValue::equals)
-      .isPresent();
-  }
-
-  protected static Optional<String> getStringValue(Expression expression) {
-    try {
-      return Optional.of(((StringLiteral) expression).trimmedQuotesValue());
-    } catch (ClassCastException e) {
-      return Optional.empty();
-    }
+  protected void checkFqn(String fqn, BiConsumer<SubscriptionContext, CallExpression> consumer) {
+    fqnCallConsumers.put(fqn, consumer);
   }
 
   /**
-   * @return Predicate which tests if expression is a string and is equal the expected value
+   * Register a consumer for multiple FQNs
    */
-  protected static Predicate<Expression> isStringValue(String expectedValue) {
-    return expression -> getStringValue(expression).filter(expectedValue::equals).isPresent();
+  protected void checkFqns(Collection<String> suffixes, BiConsumer<SubscriptionContext, CallExpression> consumer) {
+    suffixes.forEach(suffix -> checkFqn(suffix, consumer));
   }
 
-  protected static Predicate<Expression> isSensitiveMethod(SubscriptionContext ctx, String methodFqn, String argName, Predicate<Expression> sensitiveValuePredicate) {
-    return expression -> {
-      if (!isFqn(methodFqn).test(expression)) {
-        return false;
-      }
-      if (!expression.is(Tree.Kind.CALL_EXPR)) {
-        return true;
-      }
 
-      Optional<AbstractCdkResourceCheck.ArgumentTrace> argTrace = getArgument(ctx, (CallExpression) expression, argName);
-      if (argTrace.isEmpty()) {
-        return true;
-      }
-
-      return argTrace.filter(trace -> trace.hasExpression(sensitiveValuePredicate)).isPresent();
-    };
-  }
 
 }

python-checks/src/main/java/org/sonar/python/checks/cdk/AbstractS3BucketCheck.java

@@ -19,6 +19,7 @@
  */
 package org.sonar.python.checks.cdk;
 
+import java.util.function.BiConsumer;
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.tree.CallExpression;
 
@@ -27,14 +28,9 @@ public abstract class AbstractS3BucketCheck extends AbstractCdkResourceCheck {
   protected static final String S3_BUCKET_FQN = "aws_cdk.aws_s3.Bucket";
 
   @Override
-  protected String resourceFqn() {
-    return S3_BUCKET_FQN;
+  protected void registerFqnConsumer() {
+    checkFqn(S3_BUCKET_FQN, this.visitBucketConstructor());
   }
 
-  @Override
-  protected void visitResourceConstructor(SubscriptionContext ctx, CallExpression resourceConstructor) {
-    visitBucketConstructor(ctx, resourceConstructor);
-  }
-
-  abstract void visitBucketConstructor(SubscriptionContext ctx, CallExpression bucket);
+  abstract BiConsumer<SubscriptionContext, CallExpression> visitBucketConstructor();
 }

python-checks/src/main/java/org/sonar/python/checks/cdk/CdkPredicate.java

@@ -0,0 +1,107 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks.cdk;
+
+import java.util.Collection;
+import java.util.Locale;
+import java.util.Optional;
+import java.util.function.Predicate;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.Token;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.python.tree.TreeUtils;
+
+import static org.sonar.python.checks.cdk.CdkUtils.getArgument;
+
+public class CdkPredicate {
+
+  private CdkPredicate() {
+
+  }
+
+  /**
+   * @return Predicate which tests if expression is boolean literal and is set to `false`
+   */
+  public static Predicate<Expression> isFalse() {
+    return expression -> Optional.ofNullable(expression.firstToken()).map(Token::value).filter("False"::equals).isPresent();
+  }
+
+  /**
+   * @return Predicate which tests if expression is `none`
+   */
+  public static Predicate<Expression> isNone() {
+    return expression -> expression.is(Tree.Kind.NONE);
+  }
+
+  /**
+   * @return Predicate which tests if expression is a fully qualified name (FQN) and is equal the expected FQN
+   */
+  public static Predicate<Expression> isFqn(String fqnValue) {
+    return expression ->  Optional.ofNullable(TreeUtils.fullyQualifiedNameFromExpression(expression))
+      .filter(fqnValue::equals)
+      .isPresent();
+  }
+
+  /**
+   * @return Predicate which tests if expression is a fully qualified name (FQN) and part of the FQN list
+   */
+  public static Predicate<Expression> isFqnOf(Collection<String> fqnValues) {
+    return expression ->  Optional.ofNullable(TreeUtils.fullyQualifiedNameFromExpression(expression))
+      .filter(fqnValues::contains)
+      .isPresent();
+  }
+
+  /**
+   * @return Predicate which tests if expression is a string and is equal the expected value
+   */
+  public static Predicate<Expression> isString(String expectedValue) {
+    return expression -> CdkUtils.getString(expression).filter(expectedValue::equals).isPresent();
+  }
+
+  /**
+   * @return Predicate which tests if expression is a string and starts with the expected value
+   */
+  public static Predicate<Expression> startsWith(String expected) {
+    return expression -> CdkUtils.getString(expression).filter(str -> str.toLowerCase(Locale.ROOT).startsWith(expected)).isPresent();
+  }
+
+  // TODO refactor this overloaded predicate method
+  // FIXME we should not raise on a FQN which is not a method call when we want to check for a sensitive method call
+  public static Predicate<Expression> isSensitiveMethod(SubscriptionContext ctx, String methodFqn, String argName, Predicate<Expression> sensitiveValuePredicate) {
+    return expression -> {
+      if (!isFqn(methodFqn).test(expression)) {
+        return false;
+      }
+      if (!expression.is(Tree.Kind.CALL_EXPR)) {
+        return true;
+      }
+
+      Optional<CdkUtils.ExpressionTrace> argTrace = getArgument(ctx, (CallExpression) expression, argName);
+      if (argTrace.isEmpty()) {
+        return true;
+      }
+
+      return argTrace.filter(trace -> trace.hasExpression(sensitiveValuePredicate)).isPresent();
+    };
+  }
+
+}