Add function to the auditor to ensure that annotations exist in

grouping.xml.

grouping.xml defines the overall grouping/structure for annotations as
presented to clients. This CL would ensure that the annotation exists
in grouping.xml.

Bug: 1107860
Change-Id: I675f6bd0f8660faf8f16be36d515df9f04d36f6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303313
Commit-Queue: Mohamadou Bella Bah <bellabah@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794287}

docs/network_traffic_annotations.md

@@ -285,6 +285,11 @@ change list. These checks include:
 * All usages from Chrome have annotation.
 * Unique ids are unique, through history (even if an annotation gets deprecated,
   its unique id cannot be reused to keep the stats sound).
+* That the annotation appears in
+  `tools/traffic_annotation/summary/grouping.xml`. When adding a new annotation,
+  it must also be included in `grouping.xml` for reporting purposes (please
+  refer to the **Annotations Review**).
+  
 
 ### Presubmit tests
 To perform tests prior to submit, one can use the `traffic_annotation_auditor`
@@ -313,15 +318,22 @@ scope of tests have not neglected any issues.
 
 Network traffic annotations require review before landing in code and this is
 enforced through keeping a summary of annotations in
-`tools/traffic_annotation/summary/annotations.xml`.
-Once a new annotation is added, one is updated, or deleted, this file
-should also be updated. To update the file automatically, one can run
-`traffic_annotation_auditor` as specified in presubmit tests. But if it is not
-possible to do so (e.g., if you are changing the code from an unsupported
-platform or you don’t have a compiled build directory), the code can be
-submitted to the trybot and the test on trybot will tell you the required
-modifications.
-
+`tools/traffic_annotation/summary/annotations.xml`. Once a new annotation is added,
+one is updated, or deleted, this file should also be updated. To update the
+`annotations.xml` file automatically, one can run `traffic_annotation_auditor`
+as specified in presubmit tests. But if it is not possible to do so (e.g., if
+you are changing the code from an unsupported platform or you don’t have a
+compiled build directory), the code can be submitted to the trybot and the test
+on trybot will tell you the required modifications. 
+
+In order to help make external reports easier, annotation unique ids should be
+mentioned in `tools/traffic_annotation/summary/grouping.xml`. Once a new
+annotation is added, or a preexisting annotation's unique id changes, this file
+should also be updated. When adding a new annotation, make sure it is placed
+within an appropriate group of `grouping.xml`. In the rare case that none of
+the groups are appropriate, one can create a new group for the annotation; the
+arrangement of annotations and group names in `grouping.xml` may be later
+updated by a technical writer to better coincide with the external reports.
 
 ## Partial Annotations (Advanced)
 

tools/traffic_annotation/auditor/BUILD.gn

@@ -113,6 +113,7 @@ test("traffic_annotation_auditor_unittests") {
     "tests/irrelevant_file_name.txt",
     "tests/relevant_file_name_and_content.cc",
     "tests/relevant_file_name_and_content.mm",
+    "tests/test_grouping.xml",
   ]
   deps = [
     ":auditor_sources",

tools/traffic_annotation/auditor/auditor_result.cc

@@ -138,6 +138,14 @@ std::string AuditorResult::ToText() const {
           "Annotation at '%s:%i' has the following inconsistencies: %s",
           file_path_.c_str(), line_, details_[0].c_str());
 
+    case AuditorResult::Type::ERROR_MISSING_GROUPING:
+      DCHECK(!details_.empty());
+      return base::StringPrintf(
+          "Annotation at '%s:%i' with unique_id '%s' does not appear in "
+          "summary/grouping.xml. Add the annotation to an existing "
+          "group in summary/grouping.xml",
+          file_path_.c_str(), line_, details_[0].c_str());
+
     case AuditorResult::Type::ERROR_MERGE_FAILED:
       DCHECK(details_.size() == 3);
       return base::StringPrintf(
@@ -205,6 +213,9 @@ std::string AuditorResult::ToShortText() const {
       return base::StringPrintf("the following inconsistencies: %s",
                                 details_[0].c_str());
 
+    case AuditorResult::Type::ERROR_MISSING_GROUPING:
+      return base::StringPrintf("missing from summary/grouping.xml");
+
     default:
       NOTREACHED();
       return std::string();

tools/traffic_annotation/auditor/auditor_result.h

@@ -37,6 +37,8 @@ class AuditorResult {
     ERROR_MISSING_SECOND_ID,      // Annotation does not have a valid second id.
     ERROR_INCOMPLETE_ANNOTATION,  // Annotation has some missing fields.
     ERROR_INCONSISTENT_ANNOTATION,  // Annotation has some inconsistent fields.
+    ERROR_MISSING_GROUPING,         // Annotation is missing from
+                                    // summary/grouping.xml file.
     ERROR_MERGE_FAILED,            // Two annotations that are supposed to merge
                                    // cannot merge.
     ERROR_INCOMPLETED_ANNOTATION,  // A partial or [branched_] completing

tools/traffic_annotation/auditor/instance.cc

@@ -13,6 +13,7 @@
 #include "third_party/protobuf/src/google/protobuf/io/tokenizer.h"
 #include "third_party/protobuf/src/google/protobuf/text_format.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_auditor.h"
+#include "tools/traffic_annotation/auditor/traffic_annotation_exporter.h"
 
 namespace {
 
@@ -386,6 +387,19 @@ AuditorResult AnnotationInstance::IsConsistent() const {
   return AuditorResult(AuditorResult::Type::RESULT_OK);
 }
 
+AuditorResult AnnotationInstance::InGroupingXML(
+    const std::set<std::string>& grouping_annotation_unique_ids) const {
+  const std::string& unique_id = proto.unique_id();
+
+  if (grouping_annotation_unique_ids.find(unique_id) ==
+      grouping_annotation_unique_ids.end()) {
+    return AuditorResult(AuditorResult::Type::ERROR_MISSING_GROUPING,
+                         unique_id.c_str(), proto.source().file(),
+                         proto.source().line());
+  }
+  return AuditorResult(AuditorResult::Type::RESULT_OK);
+}
+
 bool AnnotationInstance::IsCompletableWith(
     const AnnotationInstance& other) const {
   if (type != AnnotationInstance::Type::ANNOTATION_PARTIAL || second_id.empty())

tools/traffic_annotation/auditor/instance.h

@@ -81,6 +81,10 @@ class AnnotationInstance : public InstanceBase {
   // Checks if annotation fields are consistent.
   AuditorResult IsConsistent() const;
 
+  // Checks if annotation appears in summary/grouping.xml
+  AuditorResult InGroupingXML(
+      const std::set<std::string>& grouping_annotation_unique_ids) const;
+
   // Checks to see if this annotation can be completed with the |other|
   // annotation, based on their unique ids, types, and extra ids. |*this| should
   // be of partial type and the |other| either COMPLETING or BRANCHED_COMPLETING

tools/traffic_annotation/auditor/tests/test_grouping.xml

+<groups>
+  <group name="Group A">
+  <sender name="Foobar Component">
+    <traffic_annotation unique_id="foobar_policy_fetcher"/>
+    <traffic_annotation unique_id="foobar_info_fetcher"/>
+  </sender>
+  <sender name="Fizzbuzz Component">
+    <traffic_annotation unique_id="fizzbuzz_handle_front_end_messages"/>
+    <traffic_annotation unique_id="fizzbuzz_hard_coded_data_source"/>
+    <traffic_annotation unique_id="fizzbuzz_http_handler"/>
+  </sender>
+  </group>
+  <group name="Group B">
+  <sender name="Widget Component">
+    <traffic_annotation unique_id="widget_grabber"/>
+  </sender>
+  </group>
+</groups>
\ No newline at end of file

tools/traffic_annotation/auditor/traffic_annotation_auditor.cc

@@ -19,6 +19,8 @@
 #include "build/build_config.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "third_party/libxml/chromium/xml_reader.h"
+#include "third_party/libxml/chromium/xml_writer.h"
 #include "third_party/re2/src/re2/re2.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_file_filter.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_id_checker.h"
@@ -67,6 +69,12 @@ const base::FilePath kExtractorScript =
         .Append(FILE_PATH_LITERAL("scripts"))
         .Append(FILE_PATH_LITERAL("extractor.py"));
 
+const base::FilePath kGroupingXmlPath =
+    base::FilePath(FILE_PATH_LITERAL("tools"))
+        .Append(FILE_PATH_LITERAL("traffic_annotation"))
+        .Append(FILE_PATH_LITERAL("summary"))
+        .Append(FILE_PATH_LITERAL("grouping.xml"));
+
 // Checks if the list of |path_filters| include the given |file_path|, or there
 // are path filters which are a folder (don't have a '.' in their name), and
 // match the file name.
@@ -634,6 +642,8 @@ void TrafficAnnotationAuditor::CheckAnnotationsContents() {
         AuditorResult result = instance.IsComplete();
         if (result.IsOK())
           result = instance.IsConsistent();
+        if (result.IsOK())
+          result = instance.InGroupingXML(grouped_annotation_unique_ids_);
         if (!result.IsOK())
           errors_.push_back(result);
         break;
@@ -671,6 +681,9 @@ void TrafficAnnotationAuditor::CheckAnnotationsContents() {
         if (result.IsOK())
           result = completed.IsConsistent();
 
+        if (result.IsOK())
+          result = completed.InGroupingXML(grouped_annotation_unique_ids_);
+
         if (result.IsOK()) {
           new_annotations.push_back(completed);
         } else {
@@ -719,6 +732,28 @@ void TrafficAnnotationAuditor::AddMissingAnnotations() {
   }
 }
 
+bool TrafficAnnotationAuditor::GetGroupingAnnotationsUniqueIDs(
+    base::FilePath grouping_xml_path,
+    std::set<std::string>* annotation_unique_ids) const {
+  XmlReader reader;
+  if (!reader.LoadFile(grouping_xml_path.MaybeAsASCII())) {
+    LOG(ERROR) << "Could not load '" << grouping_xml_path.MaybeAsASCII()
+               << "'.";
+    return false;
+  }
+
+  bool all_ok = true;
+  while (reader.Read()) {
+    if (reader.IsClosingElement() || reader.NodeName() != "traffic_annotation")
+      continue;
+
+    std::string unique_id;
+    all_ok &= reader.NodeAttribute("unique_id", &unique_id);
+    annotation_unique_ids->insert(unique_id);
+  }
+  return all_ok;
+}
+
 bool TrafficAnnotationAuditor::RunAllChecks(
     bool report_xml_updates) {
   if (exporter_.GetArchivedAnnotations().empty() &&
@@ -729,6 +764,11 @@ bool TrafficAnnotationAuditor::RunAllChecks(
   std::set<int> deprecated_ids;
   exporter_.GetDeprecatedHashCodes(&deprecated_ids);
 
+  if (!GetGroupingAnnotationsUniqueIDs(source_path_.Append(kGroupingXmlPath),
+                                       &grouped_annotation_unique_ids_)) {
+    return false;
+  }
+
   if (!path_filters_.empty())
     AddMissingAnnotations();
 

tools/traffic_annotation/auditor/traffic_annotation_auditor.h

@@ -137,6 +137,11 @@ class TrafficAnnotationAuditor {
     extracted_calls_ = calls;
   }
 
+  void SetGroupedAnnotationUniqueIDsForTesting(
+      std::set<std::string>& annotation_unique_ids) {
+    grouped_annotation_unique_ids_ = annotation_unique_ids;
+  }
+
   const std::vector<CallInstance>& extracted_calls() const {
     return extracted_calls_;
   }
@@ -159,10 +164,17 @@ class TrafficAnnotationAuditor {
 
   std::unique_ptr<google::protobuf::Message> CreateAnnotationProto();
 
+  // Produces the set of annotation unique_ids that appear in grouping.xml
+  // Returns false if grouping.xml cannot be loaded.
+  bool GetGroupingAnnotationsUniqueIDs(
+      base::FilePath grouping_xml_path,
+      std::set<std::string>* annotation_unique_ids) const;
+
  private:
   const base::FilePath source_path_;
   const base::FilePath build_path_;
   std::vector<std::string> path_filters_;
+  std::set<std::string> grouped_annotation_unique_ids_;
 
   // Variables used to dynamic the NetworkTrafficAnnotation proto.
   std::unique_ptr<google::protobuf::DescriptorPool> descriptor_pool_;

tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc

@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_exporter.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_file_filter.h"
@@ -660,9 +661,27 @@ TEST_F(TrafficAnnotationAuditorTest, CheckAllRequiredFunctionsAreAnnotated) {
 // Tests if TrafficAnnotationAuditor::CheckAnnotationsContents works as
 // expected for COMPLETE annotations. It also inherently checks
 // TrafficAnnotationAuditor::IsAnnotationComplete and
-// TrafficAnnotationAuditor::IsAnnotationConsistent.
+// TrafficAnnotationAuditor::IsAnnotationConsistent and
+// TrafficAnnotationAuditor::InGroupingXML.
 TEST_F(TrafficAnnotationAuditorTest, CheckCompleteAnnotations) {
   AnnotationInstance instance = CreateAnnotationInstanceSample();
+
+  base::FilePath grouping_xml_path =
+      tests_folder().Append(FILE_PATH_LITERAL("test_grouping.xml"));
+  std::set<std::string> annotation_unique_ids;
+  bool success = auditor().GetGroupingAnnotationsUniqueIDs(
+      grouping_xml_path, &annotation_unique_ids);
+  EXPECT_TRUE(success);
+  EXPECT_THAT(annotation_unique_ids,
+              testing::UnorderedElementsAre(
+                  "foobar_policy_fetcher", "foobar_info_fetcher",
+                  "fizzbuzz_handle_front_end_messages",
+                  "fizzbuzz_hard_coded_data_source", "fizzbuzz_http_handler",
+                  "widget_grabber"));
+  auditor().SetGroupedAnnotationUniqueIDsForTesting(annotation_unique_ids);
+  // Set unique id to be something in `tests/test_grouping.xml`.
+  instance.proto.set_unique_id("foobar_policy_fetcher");
+
   std::vector<AnnotationInstance> annotations;
   unsigned int expected_errors_count = 0;
 
@@ -942,3 +961,36 @@ TEST_F(TrafficAnnotationAuditorTest, AnnotationsXMLDifferences) {
   EXPECT_EQ(diff13, expected_diff13);
   EXPECT_EQ(diff23, expected_diff23);
 }
+
+// Tests if an 'annotation' is in 'test_grouping.xml' or not.
+TEST_F(TrafficAnnotationAuditorTest, AnnotationGrouping) {
+  AnnotationInstance instance = CreateAnnotationInstanceSample();
+  instance.type = AnnotationInstance::Type::ANNOTATION_COMPLETE;
+
+  base::FilePath grouping_xml_path =
+      tests_folder().Append(FILE_PATH_LITERAL("test_grouping.xml"));
+
+  std::set<std::string> annotation_unique_ids;
+  bool success = auditor().GetGroupingAnnotationsUniqueIDs(
+      grouping_xml_path, &annotation_unique_ids);
+
+  EXPECT_TRUE(success);
+  EXPECT_THAT(annotation_unique_ids,
+              testing::UnorderedElementsAre(
+                  "foobar_policy_fetcher", "foobar_info_fetcher",
+                  "fizzbuzz_handle_front_end_messages",
+                  "fizzbuzz_hard_coded_data_source", "fizzbuzz_http_handler",
+                  "widget_grabber"));
+
+  // Test 'annotation' with unique id "empty" is not in 'test_grouping.xml'
+  instance.proto.set_unique_id("empty");
+  AuditorResult::Type returned_type =
+      instance.InGroupingXML(annotation_unique_ids).type();
+  EXPECT_EQ(returned_type, AuditorResult::Type::ERROR_MISSING_GROUPING);
+
+  // Test 'annotation' with unique id "foobar_policy_fetcher" is in
+  // 'test_grouping.xml'
+  instance.proto.set_unique_id("foobar_policy_fetcher");
+  returned_type = instance.InGroupingXML(annotation_unique_ids).type();
+  EXPECT_EQ(returned_type, AuditorResult::Type::RESULT_OK);
+}

tools/traffic_annotation/auditor/traffic_annotation_exporter.cc

@@ -34,6 +34,12 @@ const base::FilePath kAnnotationsXmlPath =
         .Append(FILE_PATH_LITERAL("summary"))
         .Append(FILE_PATH_LITERAL("annotations.xml"));
 
+const base::FilePath kGroupingXmlPath =
+    base::FilePath(FILE_PATH_LITERAL("tools"))
+        .Append(FILE_PATH_LITERAL("traffic_annotation"))
+        .Append(FILE_PATH_LITERAL("summary"))
+        .Append(FILE_PATH_LITERAL("grouping.xml"));
+
 // Extracts annotation id from a line of XML. Expects to have the line in the
 // following format: <... id="..." .../>
 // TODO(rhalavati): Use real XML parsing.