Repository: Duke-GCB/DukeDSClient

Issue:
For Share/Deliver, register users in DukeDS as needed
The API to create a DukeDS user is at https://apidev.dataservice.duke.edu/apidocs/users_software_agents.html#auth-provider-affiliates-not_impl_new-create-or-get-user-account-for-affiliate-post, and while we can register users through this API, DukeDS will not automatically create new accounts until we call it.

Since the [share/deliver actions in DukeDSClient](https://github.com/Duke-GCB/DukeDSClient/blob/master/ddsc/ddsclient.py#L260) already have a step to look up the user by email or netid, this would be a good time to register the user account if needed.

I considered adding this step to D4S2, but D4S2 currently only deals with the UUIDs of existing users. It assumes the user has already been looked up.

Thoughts?

Discussion:

Patch:
diff --git a/ddsc/core/ddsapi.py b/ddsc/core/ddsapi.py
index f79d7bf..2039628 100644
--- a/ddsc/core/ddsapi.py
+++ b/ddsc/core/ddsapi.py
@@ -747,6 +747,26 @@ class DataServiceApi(object):
         }
         return self._put("/activities/" + activity_id, put_data)
 
+    def get_auth_providers(self):
+        """
+        List Authentication Providers Lists Authentication Providers
+        Returns an array of auth provider details
+        :return: requests.Response containing the successful result
+        """
+        return self._get_collection("/auth_providers", {})
+
+    def auth_provider_add_user(self, auth_provider_id, username):
+        """
+        Transform an institutional affiliates UID, such as a Duke NetID, to a DDS specific user identity;
+        can be used by clients prior to calling DDS APIs that require a DDS user in the request payload.
+        Returns user details. Can be safely called multiple times.
+        :param auth_provider_id: str: auth provider who supports user adding
+        :param username: str: netid we wish to register with DukeDS
+        :return: requests.Response containing the successful result
+        """
+        url = "/auth_providers/{}/affiliates/{}/dds_user/".format(auth_provider_id, username)
+        return self._post(url, {})
+
 
 class MultiJSONResponse(object):
     """
diff --git a/ddsc/ddsclient.py b/ddsc/ddsclient.py
index 48b464e..a24d036 100644
--- a/ddsc/ddsclient.py
+++ b/ddsc/ddsclient.py
@@ -166,7 +166,7 @@ class AddUserCommand(object):
         username = args.username            # username of person to give permissions, will be None if email is specified
         auth_role = args.auth_role          # type of permission(project_admin)
         project = self.remote_store.fetch_remote_project(project_name, must_exist=True, include_children=False)
-        user = self.remote_store.lookup_user_by_email_or_username(email, username)
+        user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
         self.remote_store.set_user_project_permission(project, user, auth_role)
         print(u'Gave user {} {} permissions for {}.'.format(user.full_name, auth_role, project_name))
 
@@ -191,7 +191,7 @@ class RemoveUserCommand(object):
         email = args.email                # email of person to remove permissions from (None if username specified)
         username = args.username          # username of person to remove permissions from (None if email is specified)
         project = self.remote_store.fetch_remote_project(project_name, must_exist=True, include_children=False)
-        user = self.remote_store.lookup_user_by_email_or_username(email, username)
+        user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
         self.remote_store.revoke_user_project_permission(project, user)
         print(u'Removed permissions from user {} for project {}.'.format(user.full_name, project_name))
 
@@ -218,7 +218,7 @@ class ShareCommand(object):
         username = args.username            # username of person to send email to, will be None if email is specified
         force_send = args.resend            # is this a resend so we should force sending
         auth_role = args.auth_role          # authorization role(project permissions) to give to the user
-        to_user = self.remote_store.lookup_user_by_email_or_username(email, username)
+        to_user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
         try:
             dest_email = self.service.share(project_name, to_user, force_send, auth_role)
             print("Share email message sent to " + dest_email)
@@ -256,7 +256,7 @@ class DeliverCommand(object):
         new_project_name = None
         if not skip_copy_project:
             new_project_name = self.get_new_project_name(project_name)
-        to_user = self.remote_store.lookup_user_by_email_or_username(email, username)
+        to_user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
         try:
             path_filter = PathFilter(args.include_paths, args.exclude_paths)
             dest_email = self.service.deliver(project_name, new_project_name, to_user, force_send, path_filter)


Test patch:
diff --git a/ddsc/core/remotestore.py b/ddsc/core/remotestore.py
index 6df761c..942cd88 100644
--- a/ddsc/core/remotestore.py
+++ b/ddsc/core/remotestore.py
@@ -34,7 +34,7 @@ class RemoteStore(object):
                 self._add_project_children(project)
         else:
             if must_exist:
-                raise ValueError(u'There is no project with the name {}'.format(project_name).encode('utf-8'))
+                raise NotFoundError(u'There is no project with the name {}'.format(project_name).encode('utf-8'))
         return project
 
     def fetch_remote_project_by_id(self, id):
@@ -68,10 +68,18 @@ class RemoteStore(object):
         for child in project_children.get_tree():
             project.add_child(child)
 
-    def lookup_user_by_email_or_username(self, email, username):
+    def lookup_or_register_user_by_email_or_username(self, email, username):
+        """
+        Lookup user by email or username. Only fill in one field.
+        For username it will try to register if not found.
+        :param email: str: email address of the user
+        :param username: netid of the user to find
+        :return: RemoteUser
+        """
         if username:
-            return self.lookup_user_by_username(username)
+            return self.get_or_register_user_by_username(username)
         else:
+            # API doesn't support registering user by email address yet
             return self.lookup_user_by_email(email)
 
     def lookup_user_by_name(self, full_name):
@@ -85,27 +93,72 @@ class RemoteStore(object):
         results = json_data['results']
         found_cnt = len(results)
         if found_cnt == 0:
-            raise ValueError("User not found:" + full_name)
+            raise NotFoundError("User not found:" + full_name)
         elif found_cnt > 1:
             raise ValueError("Multiple users with name:" + full_name)
         user = RemoteUser(results[0])
         if user.full_name.lower() != full_name.lower():
-            raise ValueError("User not found:" + full_name)
+            raise NotFoundError("User not found:" + full_name)
         return user
 
     def lookup_user_by_username(self, username):
         """
         Finds the single user who has this username or raises ValueError.
         :param username: str username we are looking for
-        :return: RemoteUser user we found
+        :return: RemoteUser: user we found
         """
         matches = [user for user in self.fetch_all_users() if user.username == username]
         if not matches:
-            raise ValueError('Username not found: {}.'.format(username))
+            raise NotFoundError('Username not found: {}.'.format(username))
         if len(matches) > 1:
             raise ValueError('Multiple users with same username found: {}.'.format(username))
         return matches[0]
 
+    def get_or_register_user_by_username(self, username):
+        """
+        Try to lookup user by username. If not found try registering the user.
+        :param username: str: username to lookup
+        :return: RemoteUser: user we found
+        """
+        try:
+            return self.lookup_user_by_username(username)
+        except NotFoundError:
+            return self.register_user_by_username(username)
+
+    def register_user_by_username(self, username):
+        """
+        Tries to register user with the first non-deprecated auth provider.
+        Raises ValueError if the data service doesn't have any non-deprecated providers.
+        :param username: str: netid of the user we are trying to register
+        :return: RemoteUser: user that was created for our netid
+        """
+        current_providers = [prov.id for prov in self.get_auth_providers() if not prov.is_deprecated]
+        if not current_providers:
+            raise ValueError("Unable to register user: no non-deprecated providers found!")
+        auth_provider_id = current_providers[0]
+        return self._register_user_by_username(auth_provider_id, username)
+
+    def _register_user_by_username(self, auth_provider_id, username):
+        """
+        Tries to register a user who has a valid netid but isn't registered with DukeDS yet under auth_provider_id.
+        :param auth_provider_id: str: id from RemoteAuthProvider to use for registering
+        :param username: str: netid of the user we are trying to register
+        :return: RemoteUser: user that was created for our netid
+        """
+        user_json = self.data_service.auth_provider_add_user(auth_provider_id, username).json()
+        return RemoteUser(user_json)
+
+    def get_auth_providers(self):
+        """
+        Return the list of authorization providers.
+        :return: [RemoteAuthProvider]: list of remote auth providers
+        """
+        providers = []
+        response = self.data_service.get_auth_providers().json()
+        for data in response['results']:
+            providers.append(RemoteAuthProvider(data))
+        return providers
+
     def lookup_user_by_email(self, email):
         """
         Finds the single user who has this email or raises ValueError.
@@ -458,3 +511,24 @@ class RemoteProjectChildren(object):
                 file = RemoteFile(child_data, parent_path)
                 children.append(file)
         return children
+
+
+class RemoteAuthProvider(object):
+    def __init__(self, json_data):
+        """
+        Set properties based on json_data.
+        :param json_data: dict JSON data containing auth_role info
+        """
+        self.id = json_data['id']
+        self.service_id = json_data['service_id']
+        self.name = json_data['name']
+        self.is_deprecated = json_data['is_deprecated']
+        self.is_default = json_data['is_default']
+        self.is_deprecated = json_data['is_deprecated']
+        self.login_initiation_url = json_data['login_initiation_url']
+
+
+class NotFoundError(Exception):
+    def __init__(self, message):
+        Exception.__init__(self, message)
+        self.message = message
diff --git a/ddsc/core/tests/test_ddsapi.py b/ddsc/core/tests/test_ddsapi.py
index 0bb219e..5b19adc 100644
--- a/ddsc/core/tests/test_ddsapi.py
+++ b/ddsc/core/tests/test_ddsapi.py
@@ -10,6 +10,12 @@ def fake_response_with_pages(status_code, json_return_value, num_pages=1):
     return mock_response
 
 
+def fake_response(status_code, json_return_value):
+    mock_response = MagicMock(status_code=status_code, headers={})
+    mock_response.json.return_value = json_return_value
+    return mock_response
+
+
 class TestMultiJSONResponse(TestCase):
     """
     Tests that we can merge multiple JSON responses arrays with a given name(merge_array_field_name).
@@ -187,3 +193,43 @@ class TestDataServiceApi(TestCase):
         api = DataServiceApi(auth=None, url="something.com/v1/", http=mock_requests)
         resp = api._get_single_page(url_suffix='stuff', data={}, content_type=ContentType.json, page_num=1)
         self.assertEqual(True, resp.json()['ok'])
+
+    def test_get_auth_providers(self):
+        provider = {
+            "id": "aca35ba3-a44a-47c2-8b3b-afe43a88360d",
+            "service_id": "cfde039d-f550-47e7-833c-9ebc4e257847",
+            "name": "Duke Authentication Service",
+            "is_deprecated": True,
+            "is_default": True,
+            "login_initiation_url": "https://someurl"
+        }
+        json_results = {
+            "results": [
+                provider
+            ]
+        }
+        mock_requests = MagicMock()
+        mock_requests.get.side_effect = [
+            fake_response_with_pages(status_code=200, json_return_value=json_results, num_pages=1)
+        ]
+        api = DataServiceApi(auth=None, url="something.com/v1", http=mock_requests)
+        result = api.get_auth_providers()
+        self.assertEqual(200, result.status_code)
+        self.assertEqual(json_results, result.json())
+        self.assertEqual('something.com/v1/auth_providers', mock_requests.get.call_args_list[0][0][0])
+
+    def test_auth_provider_add_user(self):
+        user = {
+            "id": "abc4e9-9987-47eb-bb4e-19f0203efbf6",
+            "username": "joe",
+        }
+        mock_requests = MagicMock()
+        mock_requests.post.side_effect = [
+            fake_response(status_code=200, json_return_value=user)
+        ]
+        api = DataServiceApi(auth=None, url="something.com/v1", http=mock_requests)
+        result = api.auth_provider_add_user('123', "joe")
+        self.assertEqual(200, result.status_code)
+        self.assertEqual(user, result.json())
+        expected_url = 'something.com/v1/auth_providers/123/affiliates/joe/dds_user/'
+        self.assertEqual(expected_url, mock_requests.post.call_args_list[0][0][0])
diff --git a/ddsc/core/tests/test_remotestore.py b/ddsc/core/tests/test_remotestore.py
index 524fcda..5a3a7fb 100644
--- a/ddsc/core/tests/test_remotestore.py
+++ b/ddsc/core/tests/test_remotestore.py
@@ -1,10 +1,12 @@
 import json
 from unittest import TestCase
-
+from mock import MagicMock
+from mock.mock import patch
 from ddsc.core.remotestore import RemoteProject, RemoteFolder, RemoteFile, RemoteUser
 from ddsc.core.remotestore import RemoteStore
 from ddsc.core.remotestore import RemoteAuthRole
 from ddsc.core.remotestore import RemoteProjectChildren
+from ddsc.core.remotestore import RemoteAuthProvider
 
 
 class TestProjectFolderFile(TestCase):
@@ -544,3 +546,76 @@ class TestReadRemoteHash(TestCase):
         hash_info = RemoteFile.get_hash_from_upload(upload)
         self.assertEqual(hash_info["value"], "aabbcc")
         self.assertEqual(hash_info["algorithm"], "md5")
+
+
+class TestRemoteAuthProvider(TestCase):
+    def setUp(self):
+        self.provider_data1 = {
+            "id": "aca35ba3-a44a-47c2-8b3b-afe43a88360d",
+            "service_id": "cfde039d-f550-47e7-833c-9ebc4e257847",
+            "name": "Duke Authentication Service",
+            "is_deprecated": False,
+            "is_default": True,
+            "login_initiation_url": "https://someurl"
+        }
+
+    def test_constructor(self):
+        auth_provider = RemoteAuthProvider(self.provider_data1)
+        self.assertEqual(auth_provider.id, "aca35ba3-a44a-47c2-8b3b-afe43a88360d")
+        self.assertEqual(auth_provider.service_id, "cfde039d-f550-47e7-833c-9ebc4e257847")
+        self.assertEqual(auth_provider.name, "Duke Authentication Service")
+        self.assertEqual(auth_provider.is_deprecated, False)
+        self.assertEqual(auth_provider.is_default, True)
+        self.assertEqual(auth_provider.login_initiation_url, "https://someurl")
+
+    @patch("ddsc.core.remotestore.DataServiceApi")
+    def test_get_auth_providers(self, mock_data_service_api):
+        response = MagicMock()
+        response.json.return_value = {
+            "results": [self.provider_data1]
+        }
+        mock_data_service_api().get_auth_providers.return_value = response
+        remote_store = RemoteStore(MagicMock())
+        providers = remote_store.get_auth_providers()
+        self.assertEqual(len(providers), 1)
+        self.assertEqual(providers[0].name, "Duke Authentication Service")
+
+    @patch("ddsc.core.remotestore.DataServiceApi")
+    def test_register_user_by_username(self, mock_data_service_api):
+        providers_response = MagicMock()
+        providers_response.json.return_value = {
+            "results": [self.provider_data1]
+        }
+        add_user_response = MagicMock()
+        add_user_response.json.return_value = {
+            "id": "123",
+            "username": "joe",
+            "full_name": "Joe Shoe",
+            "email": "",
+        }
+        mock_data_service_api().get_auth_providers.return_value = providers_response
+        mock_data_service_api().auth_provider_add_user.return_value = add_user_response
+        remote_store = RemoteStore(MagicMock())
+        user = remote_store.register_user_by_username("joe")
+        self.assertEqual(user.id, "123")
+        self.assertEqual(user.username, "joe")
+        mock_data_service_api().auth_provider_add_user.assert_called_with(self.provider_data1['id'], 'joe')
+
+    @patch("ddsc.core.remotestore.DataServiceApi")
+    def test_register_user_by_username_with_no_default_provider(self, mock_data_service_api):
+        providers_response = MagicMock()
+        providers_response.json.return_value = {
+            "results": []
+        }
+        add_user_response = MagicMock()
+        add_user_response.json.return_value = {
+            "id": "123",
+            "username": "joe",
+            "full_name": "Joe Shoe",
+            "email": "",
+        }
+        mock_data_service_api().get_auth_providers.return_value = providers_response
+        mock_data_service_api().auth_provider_add_user.return_value = add_user_response
+        remote_store = RemoteStore(MagicMock())
+        with self.assertRaises(ValueError):
+            remote_store.register_user_by_username("joe")