Check for upload permission based on google group (#4562)

Although the IssueTracker component create controls are locked down to
this group, its possible for folks to accidentally migrate bugs into
this component. To prevent that from happening, check for the reporter's
membership before proceeding with the upload. It is a bit hacky i admit.

Author: pgrace-google

src/clusterfuzz/_internal/cron/external_testcase_reader.py

@@ -16,6 +16,7 @@
 import datetime
 import re
 
+from google.cloud import storage
 import requests
 
 from appengine.libs import form
@@ -31,7 +32,18 @@
 ISSUETRACKER_WONTFIX_STATE = 'NOT_REPRODUCIBLE'
 
 
-def close_issue_if_invalid(upload_request, attachment_info, description):
+def get_vrp_uploaders():
+  """Checks whether the given reporter has permission to upload."""
+  # TODO(pgrace) Add this to a YAML file.
+  storage_client = storage.Client()
+  bucket = storage_client.bucket('clusterfuzz-vrp-uploaders')
+  blob = bucket.blob('vrp-uploaders')
+  members = blob.download_as_string().decode('utf-8').splitlines()[0].split(',')
+  return members
+
+
+def close_issue_if_invalid(upload_request, attachment_info, description,
+                           vrp_uploaders):
   """Closes any invalid upload requests with a helpful message."""
   comment_message = (
       'Hello, this issue is automatically closed. Please file a new bug after'
@@ -42,7 +54,12 @@ def close_issue_if_invalid(upload_request, attachment_info, description):
   if upload_request.id == 373893311:
     return False
 
-  # TODO(pgrace) Add secondary check for authorized reporters.
+  if not upload_request.reporter in vrp_uploaders:
+    comment_message += (
+        'You are not authorized to submit testcases to Clusterfuzz.'
+        ' If you believe you should be, please reach out to'
+        ' [email protected] for assistance.\n')
+    invalid = True
 
   # Issue must have exactly one attachment.
   if len(attachment_info) != 1:
@@ -63,7 +80,7 @@ def close_issue_if_invalid(upload_request, attachment_info, description):
 
   # Issue must have valid flags as the description.
   flag_format = re.compile(r'^([ ]?\-\-[A-Za-z\-\_]*){50}$')
-  if flag_format.match(description):
+  if description and flag_format.match(description):
     comment_message += (
         'Please provide flags in the format: "--test_flag_one --testflagtwo",\n'
     )
@@ -72,7 +89,7 @@ def close_issue_if_invalid(upload_request, attachment_info, description):
   if invalid:
     comment_message += (
         '\nPlease see the new bug template for more information on how to use'
-        'Clusterfuzz direct uploads.')
+        ' Clusterfuzz direct uploads.')
     upload_request.status = ISSUETRACKER_WONTFIX_STATE
     upload_request.save(new_comment=comment_message, notify=True)
 
@@ -142,6 +159,12 @@ def handle_testcases(tracker):
       query_filters=['componentid:1600865', 'id:373893311'],
       only_open=True)
 
+  if len(issues) == 0:
+    return
+
+  # TODO(pgrace) Cache in redis.
+  vrp_uploaders = get_vrp_uploaders()
+
   # TODO(pgrace) Implement rudimentary rate limiting.
 
   for issue in issues:
@@ -154,7 +177,8 @@ def handle_testcases(tracker):
     # Close out invalid bugs.
     attachment_metadata = tracker.get_attachment_metadata(issue.id)
     commandline_flags = tracker.get_description(issue.id)
-    if close_issue_if_invalid(issue, attachment_metadata, commandline_flags):
+    if close_issue_if_invalid(issue, attachment_metadata, commandline_flags,
+                              vrp_uploaders):
       helpers.log('Closing issue {issue_id} as it is invalid', issue.id)
       continue
 

src/clusterfuzz/_internal/tests/appengine/handlers/cron/external_testcase_reader_test.py

@@ -32,108 +32,131 @@
 }
 
 
+@mock.patch.object(
+    external_testcase_reader,
+    'get_vrp_uploaders',
+    return_value=['[email protected]'],
+    autospec=True)
+@mock.patch.object(external_testcase_reader, 'submit_testcase', autospec=True)
+@mock.patch.object(
+    external_testcase_reader, 'close_issue_if_invalid', autospec=True)
 class ExternalTestcaseReaderTest(unittest.TestCase):
-  """external_testcase_reader tests."""
+  """external_testcase_reader handle main function tests."""
 
-  def setUp(self):
-    self.mock_basic_issue = mock.MagicMock()
-    self.mock_basic_issue.created_time = '2024-06-25T01:29:30.021Z'
-    self.mock_basic_issue.status = 'NEW'
-    external_testcase_reader.submit_testcase = mock.MagicMock()
-
-  def test_handle_testcases(self):
+  def test_handle_testcases(self, mock_close_issue_if_invalid,
+                            mock_submit_testcase, _):
     """Test a basic handle_testcases where issue is fit for submission."""
+    mock_close_issue_if_invalid.return_value = False
     mock_it = mock.create_autospec(issue_tracker.IssueTracker)
-    mock_it.find_issues_with_filters.return_value = [self.mock_basic_issue]
-    external_testcase_reader.close_issue_if_invalid = mock.MagicMock()
-    external_testcase_reader.close_issue_if_invalid.return_value = False
+    basic_issue = mock.MagicMock()
+    basic_issue.reporter.return_value = '[email protected]'
+    mock_it.find_issues_with_filters.return_value = [basic_issue]
 
     external_testcase_reader.handle_testcases(mock_it)
-    external_testcase_reader.close_issue_if_invalid.assert_called_once()
+
+    mock_close_issue_if_invalid.assert_called_once()
     mock_it.get_attachment.assert_called_once()
-    external_testcase_reader.submit_testcase.assert_called_once()
+    mock_submit_testcase.assert_called_once()
 
-  def test_handle_testcases_invalid(self):
+  def test_handle_testcases_invalid(self, mock_close_issue_if_invalid,
+                                    mock_submit_testcase, _):
     """Test a basic handle_testcases where issue is invalid."""
+    mock_close_issue_if_invalid.return_value = True
     mock_it = mock.create_autospec(issue_tracker.IssueTracker)
-    mock_it.find_issues_with_filters.return_value = [self.mock_basic_issue]
-    external_testcase_reader.close_issue_if_invalid = mock.MagicMock()
-    external_testcase_reader.close_issue_if_invalid.return_value = True
+    basic_issue = mock.MagicMock()
+    basic_issue.reporter.return_value = '[email protected]'
+    mock_it.find_issues_with_filters.return_value = [basic_issue]
 
     external_testcase_reader.handle_testcases(mock_it)
-    external_testcase_reader.close_issue_if_invalid.assert_called_once()
-    mock_it.get_attachment.assert_not_called()
-    external_testcase_reader.submit_testcase.assert_not_called()
 
-  def test_handle_testcases_not_reproducible(self):
-    """Test a basic handle_testcases where issue is not reprodiclbe."""
+    mock_close_issue_if_invalid.assert_called_once()
+    mock_it.get_attachment.assert_not_called()
+    mock_submit_testcase.assert_not_called()
+
+  @mock.patch.object(
+      external_testcase_reader,
+      'close_issue_if_not_reproducible',
+      autospec=True)
+  def test_handle_testcases_not_reproducible(
+      self, mock_repro, mock_close_issue_if_invalid, mock_submit_testcase, _):
+    """Test a basic handle_testcases where issue is not reproducible."""
+    mock_repro.return_value = True
     mock_it = mock.create_autospec(issue_tracker.IssueTracker)
-    mock_it.find_issues_with_filters.return_value = [self.mock_basic_issue]
-    external_testcase_reader.close_issue_if_not_reproducible = mock.MagicMock()
-    external_testcase_reader.close_issue_if_not_reproducible.return_value = True
-    external_testcase_reader.close_issue_if_invalid = mock.MagicMock()
+    basic_issue = mock.MagicMock()
+    basic_issue.reporter.return_value = '[email protected]'
+    mock_it.find_issues_with_filters.return_value = [basic_issue]
 
     external_testcase_reader.handle_testcases(mock_it)
-    external_testcase_reader.close_issue_if_invalid.assert_not_called()
+
+    mock_close_issue_if_invalid.assert_not_called()
     mock_it.get_attachment.assert_not_called()
-    external_testcase_reader.submit_testcase.assert_not_called()
+    mock_submit_testcase.assert_not_called()
 
-  def test_handle_testcases_no_issues(self):
+  def test_handle_testcases_no_issues(self, mock_close_issue_if_invalid,
+                                      mock_submit_testcase, _):
     """Test a basic handle_testcases that returns no issues."""
     mock_it = mock.create_autospec(issue_tracker.IssueTracker)
     mock_it.find_issues_with_filters.return_value = []
-    external_testcase_reader.close_issue_if_invalid = mock.MagicMock()
 
     external_testcase_reader.handle_testcases(mock_it)
-    external_testcase_reader.close_issue_if_invalid.assert_not_called()
+
+    mock_close_issue_if_invalid.assert_not_called()
     mock_it.get_attachment.assert_not_called()
-    external_testcase_reader.submit_testcase.assert_not_called()
+    mock_submit_testcase.assert_not_called()
 
-  def test_close_issue_if_not_reproducible_true(self):
-    """Test a basic close_issue_if_invalid with valid flags."""
-    external_testcase_reader.filed_one_day_ago = mock.MagicMock()
-    external_testcase_reader.filed_one_day_ago.return_value = True
-    self.mock_basic_issue.status = 'ACCEPTED'
-    self.assertEqual(
-        True,
-        external_testcase_reader.close_issue_if_not_reproducible(
-            self.mock_basic_issue))
+
+class ExternalTestcaseReaderInvalidIssueTest(unittest.TestCase):
+  """external_testcase_reader close_issue_if_invalid tests."""
+
+  def setUp(self):
+    self.mock_basic_issue = mock.MagicMock()
+    self.mock_basic_issue.created_time = '2024-06-25T01:29:30.021Z'
+    self.mock_basic_issue.status = 'NEW'
+    self.mock_basic_issue.reporter = '[email protected]'
 
   def test_close_issue_if_invalid_basic(self):
     """Test a basic close_issue_if_invalid with valid flags."""
     attachment_info = [BASIC_ATTACHMENT]
     description = '--flag-one --flag_two'
-    self.assertEqual(
-        False,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(False, actual)
 
   def test_close_issue_if_invalid_no_flag(self):
     """Test a basic close_issue_if_invalid with no flags."""
     attachment_info = [BASIC_ATTACHMENT]
     description = ''
-    self.assertEqual(
-        False,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(False, actual)
 
   def test_close_issue_if_invalid_too_many_attachments(self):
     """Test close_issue_if_invalid with too many attachments."""
     attachment_info = [BASIC_ATTACHMENT, BASIC_ATTACHMENT]
     description = ''
-    self.assertEqual(
-        True,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(True, actual)
 
   def test_close_issue_if_invalid_no_attachments(self):
     """Test close_issue_if_invalid with no attachments."""
     attachment_info = []
     description = ''
-    self.assertEqual(
-        True,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(True, actual)
 
   def test_close_issue_if_invalid_invalid_upload(self):
     """Test close_issue_if_invalid with an invalid upload."""
@@ -146,10 +169,12 @@ def test_close_issue_if_invalid_invalid_upload(self):
         'etag': 'TXpjek9Ea3pNekV4TFRZd01USTNOalk0TFRjNE9URTROVFl4TlE9PQ=='
     }]
     description = ''
-    self.assertEqual(
-        True,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(True, actual)
 
   def test_close_issue_if_invalid_invalid_content_type(self):
     """Test close_issue_if_invalid with an invalid content type."""
@@ -164,7 +189,41 @@ def test_close_issue_if_invalid_invalid_content_type(self):
         'etag': 'TXpjek9Ea3pNekV4TFRZd01USTNOalk0TFRjNE9URTROVFl4TlE9PQ=='
     }]
     description = ''
-    self.assertEqual(
-        True,
-        external_testcase_reader.close_issue_if_invalid(
-            self.mock_basic_issue, attachment_info, description))
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description,
+        ['[email protected]'])
+
+    self.assertEqual(True, actual)
+
+  def test_close_issue_if_invalid_no_permission(self):
+    """Test close_issue_if_invalid with an no upload permission."""
+    attachment_info = [BASIC_ATTACHMENT]
+    description = ''
+    actual = external_testcase_reader.close_issue_if_invalid(
+        self.mock_basic_issue, attachment_info, description, [])
+
+    self.assertEqual(True, actual)
+
+
+class ExternalTestcaseReaderPermissionTest(unittest.TestCase):
+  """external_testcase_reader get_vrp_uploaders tests."""
+
+  def test_get_vrp_uploaders(self):
+    """Test get_vrp_uploaders."""
+    with mock.patch(
+        'src.clusterfuzz._internal.cron.external_testcase_reader.storage.Client'
+    ) as mock_storage:
+      mock_storage.return_value = mock.MagicMock()
+      mock_bucket = mock.MagicMock()
+      mock_storage.return_value.bucket.return_value = mock_bucket
+      mock_blob = mock.MagicMock()
+      mock_bucket.blob.return_value = mock_blob
+      mock_blob.download_as_string.return_value = "[email protected],[email protected]".encode(
+          'utf-8')
+
+      actual = external_testcase_reader.get_vrp_uploaders()
+      mock_storage.return_value.bucket.assert_called_once_with(
+          'clusterfuzz-vrp-uploaders')
+      mock_bucket.blob.assert_called_once_with('vrp-uploaders')
+      self.assertEqual(actual,
+                       ['[email protected]', '[email protected]'])