Merge pull request #299 from Duke-GCB/297-prevent-invalid-share-users

prevent sharing with delivery recipient

Author: johnbradley

ddsc/ddsclient.py

@@ -18,6 +18,7 @@
 from ddsc.sdk.client import Client
 
 NO_PROJECTS_FOUND_MESSAGE = 'No projects found.'
+INVALID_DELIVERY_RECIPIENT_MSG = 'Delivery recipient cannot be a share user. Remove recipient from --share-users and try again.'
 TWO_SECONDS = 2
 
 
@@ -128,19 +129,14 @@ def make_user_list(self, emails, usernames):
         :return: [RemoteUser]: details about any users referenced the two parameters
         """
         to_users = []
-        remaining_emails = [] if not emails else list(emails)
-        remaining_usernames = [] if not usernames else list(usernames)
-        for user in self.remote_store.fetch_users():
-            if user.email in remaining_emails:
+        if emails:
+            for email in emails:
+                user = self.remote_store.get_or_register_user_by_email(email)
                 to_users.append(user)
-                remaining_emails.remove(user.email)
-            elif user.username in remaining_usernames:
+        if usernames:
+            for username in usernames:
+                user = self.remote_store.get_or_register_user_by_username(username)
                 to_users.append(user)
-                remaining_usernames.remove(user.username)
-        if remaining_emails or remaining_usernames:
-            unable_to_find_users = ','.join(remaining_emails + remaining_usernames)
-            msg = "Unable to find users for the following email/usernames: {}".format(unable_to_find_users)
-            raise ValueError(msg)
         return to_users
 
 
@@ -347,6 +343,8 @@ def run(self, args):
         if copy_project:
             new_project_name = self.get_new_project_name(project.name)
         to_user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
+        if to_user.id in [share_user.id for share_user in share_users]:
+            raise ValueError(INVALID_DELIVERY_RECIPIENT_MSG)
         try:
             path_filter = PathFilter(args.include_paths, args.exclude_paths)
             dest_email = self.service.deliver(project, new_project_name, to_user, share_users,

ddsc/tests/test_ddsclient.py

@@ -1,7 +1,8 @@
 from __future__ import absolute_import
 from unittest import TestCase
 from ddsc.ddsclient import BaseCommand, UploadCommand, ListCommand, DownloadCommand, ClientCommand, MoveCommand
-from ddsc.ddsclient import ShareCommand, DeliverCommand, InfoCommand, read_argument_file_contents
+from ddsc.ddsclient import ShareCommand, DeliverCommand, InfoCommand, read_argument_file_contents, \
+    INVALID_DELIVERY_RECIPIENT_MSG
 from mock import patch, MagicMock, Mock, call, ANY
 
 
@@ -36,10 +37,13 @@ def test_fetch_project(self, mock_remote_store):
     def test_make_user_list(self, mock_remote_store):
         mock_config = MagicMock()
         base_cmd = BaseCommand(mock_config)
-        mock_remote_store.return_value.fetch_users.return_value = [
+        mock_remote_store.return_value.get_or_register_user_by_username.side_effect = [
             Mock(username='joe', email='joe@joe.joe'),
             Mock(username='bob', email='bob@bob.bob'),
-            Mock(username='tim', email='tim@tim.tim'),
+        ]
+        mock_remote_store.return_value.get_or_register_user_by_email.side_effect = [
+            Mock(username='joe', email='joe@joe.joe'),
+            ValueError("Invalid")
         ]
 
         # Find users by username
@@ -48,15 +52,22 @@ def test_make_user_list(self, mock_remote_store):
             'bob'
         ])
         self.assertEqual([user.email for user in results], ['joe@joe.joe', 'bob@bob.bob'])
+        mock_remote_store.return_value.get_or_register_user_by_username.assert_has_calls([
+            call("joe"),
+            call("bob")
+        ])
 
         # Find users by email
         results = base_cmd.make_user_list(emails=['joe@joe.joe'], usernames=None)
         self.assertEqual([user.username for user in results], ['joe'])
+        mock_remote_store.return_value.get_or_register_user_by_email.assert_has_calls([
+            call("joe@joe.joe")
+        ])
 
-        # Should get an error for invalid emails or usernames
+        # Should get an error for invalid users
         with self.assertRaises(ValueError) as raisedError:
-            base_cmd.make_user_list(emails=['no@no.no'], usernames=['george'])
-        self.assertEqual('Unable to find users for the following email/usernames: no@no.no,george',
+            base_cmd.make_user_list(emails=['no@no.no'], usernames=[])
+        self.assertEqual('Invalid',
                          str(raisedError.exception))
 
 
@@ -278,6 +289,62 @@ def test_run_message(self, mock_d4s2_project, mock_remote_store):
             args, kwargs = mock_remote_store.return_value.fetch_remote_project.call_args
             self.assertEqual('456', args[0].get_id_or_raise())
 
+    @patch('ddsc.ddsclient.RemoteStore')
+    @patch('ddsc.ddsclient.D4S2Project')
+    def test_run_share_users_good(self, mock_d4s2_project, mock_remote_store):
+        mock_to_user = Mock()
+        mock_share_user = Mock()
+        mock_remote_store.return_value.lookup_or_register_user_by_email_or_username.return_value = mock_to_user
+        mock_remote_store.return_value.get_or_register_user_by_username.return_value = mock_share_user
+        cmd = DeliverCommand(MagicMock())
+        cmd.get_new_project_name = Mock()
+        cmd.get_new_project_name.return_value = 'NewProjectName'
+        myargs = Mock(project_name='mouse',
+                      project_id=None,
+                      email=None,
+                      resend=False,
+                      username='joe123',
+                      share_usernames=['joe456'],
+                      share_emails=[],
+                      copy_project=True,
+                      include_paths=None,
+                      exclude_paths=None,
+                      msg_file=None)
+        cmd.run(myargs)
+        mock_d4s2_project.return_value.deliver.assert_called_with(
+            mock_remote_store.return_value.fetch_remote_project.return_value,
+            'NewProjectName',
+            mock_to_user,
+            [mock_share_user],
+            False,
+            ANY,
+            ''
+        )
+
+    @patch('ddsc.ddsclient.RemoteStore')
+    @patch('ddsc.ddsclient.D4S2Project')
+    def test_run_share_users_invalid(self, mock_d4s2_project, mock_remote_store):
+        mock_remote_user = Mock()
+        mock_remote_store.return_value.lookup_or_register_user_by_email_or_username.return_value = mock_remote_user
+        mock_remote_store.return_value.get_or_register_user_by_username.return_value = mock_remote_user
+        cmd = DeliverCommand(MagicMock())
+        cmd.get_new_project_name = Mock()
+        cmd.get_new_project_name.return_value = 'NewProjectName'
+        myargs = Mock(project_name='mouse',
+                      project_id=None,
+                      email=None,
+                      resend=False,
+                      username='joe123',
+                      share_usernames=['joe123'],
+                      share_emails=[],
+                      copy_project=True,
+                      include_paths=None,
+                      exclude_paths=None,
+                      msg_file=None)
+        with self.assertRaises(ValueError) as raised_exception:
+            cmd.run(myargs)
+        self.assertEqual(str(raised_exception.exception), INVALID_DELIVERY_RECIPIENT_MSG)
+
 
 class TestDDSClient(TestCase):
     def test_read_argument_file_contents(self):