Check for upload permission based on google group

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]'])