Fixes invocation policy to correctly apply allow/disallow policies to flags

which have converters that return lists. The problem was that the policy might,
say, disallow values ["a", "b"], and a flag --foo might have a converter which
takes a string and splits it by commas to produce a list. The options parser
would apply the converter to --foo=a,b to produce ["a", "b"], but invocation
policy would compare each element of the policy to the list itself, which will
never work.

--
PiperOrigin-RevId: 142297177
MOS_MIGRATED_REVID=142297177

2 files changed, 92 insertions, 5 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
index a5d28cf181..c2b917c6fd 100644
--- a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
@@ -373,7 +373,16 @@ public final class InvocationPolicyEnforcer {
       // can be arbitrarily complex.
       Set<Object> convertedPolicyValues = Sets.newHashSet();
       for (String value : policyValues) {
-        convertedPolicyValues.add(optionDescription.getConverter().convert(value));
+        Object convertedValue = optionDescription.getConverter().convert(value);
+        // Some converters return lists, and if the flag is a repeatable flag, the items in the
+        // list from the converter should be added, and not the list itself. Otherwise the items
+        // from invocation policy will be compared to lists, which will never work.
+        // See OptionsParserImpl.ParsedOptionEntry.addValue.
+        if (optionDescription.getAllowMultiple() && convertedValue instanceof List<?>) {
+          convertedPolicyValues.addAll((List<?>) convertedValue);
+        } else {
+          convertedPolicyValues.add(optionDescription.getConverter().convert(value));
+        }
       }
 
       // Check that if the default value of the flag is disallowed by the policy, that the policy
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
index 6202caec57..02bd62da98 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
@@ -25,25 +25,41 @@ import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.flags.CommandNameCache;
 import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
-
+import java.io.ByteArrayOutputStream;
+import java.util.Arrays;
+import java.util.List;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.io.ByteArrayOutputStream;
-import java.util.List;
-
 @RunWith(JUnit4.class)
 public class InvocationPolicyEnforcerTest {
 
   public static final String STRING_FLAG_DEFAULT = "test string default";
 
+  /** Test converter that splits a string by commas to produce a list. */
+  public static class ToListConverter implements Converter<List<String>> {
+
+    public ToListConverter() {}
+
+    @Override
+    public List<String> convert(String input) throws OptionsParsingException {
+      return Arrays.asList(input.split(","));
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a list of strings";
+    }
+  }
+  
   public static class TestOptions extends OptionsBase {
 
     /*
@@ -64,6 +80,18 @@ public class InvocationPolicyEnforcerTest {
     public List<String> testMultipleString;
 
     /*
+     * Flags with converters that return lists
+     */
+
+    @Option(
+      name = "test_list_converters",
+      defaultValue = "",
+      allowMultiple = true,
+      converter = ToListConverter.class
+    )
+    public List<String> testListConverters;
+
+    /*
      * Expansion flags
      */
 
@@ -877,6 +905,31 @@ public class InvocationPolicyEnforcerTest {
     }
   }
 
+  @Test
+  public void testAllowValuesDisallowsListConverterFlagValues() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_list_converters")
+        .getAllowValuesBuilder()
+        .addAllowedValues("a");
+
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    parser.parse("--test_list_converters=a,b,c");
+
+    TestOptions testOptions = getTestOptions();
+    assertEquals(Arrays.asList("a", "b", "c"), testOptions.testListConverters);
+
+    try {
+      enforcer.enforce(parser, "build");
+      fail();
+    } catch (OptionsParsingException e) {
+      assertThat(e.getMessage())
+          .contains(
+              "Flag value 'b' for flag 'test_list_converters' is not allowed by invocation policy");
+    }
+  }
+  
   /*************************************************************************************************
    * Tests for DisallowValues
    ************************************************************************************************/
@@ -1081,6 +1134,31 @@ public class InvocationPolicyEnforcerTest {
       // expected.
     }
   }
+
+  @Test
+  public void testDisallowValuesDisallowsListConverterFlag() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_list_converters")
+        .getDisallowValuesBuilder()
+        .addDisallowedValues("a");
+
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    parser.parse("--test_list_converters=a,b,c");
+
+    TestOptions testOptions = getTestOptions();
+    assertEquals(Arrays.asList("a", "b", "c"), testOptions.testListConverters);
+
+    try {
+      enforcer.enforce(parser, "build");
+      fail();
+    } catch (OptionsParsingException e) {
+      assertThat(e.getMessage())
+          .contains(
+              "Flag value 'a' for flag 'test_list_converters' is not allowed by invocation policy");
+    }
+  }
   
   /*************************************************************************************************
    * Other tests