Repository: TACC/agavepy

Issue:
Re-implement client creation to properly manage credentials
Currently to create an agave oauth client one has to specify username and password at the moment an agave object is instantiated, see #41 .
Will re-implement the client creation to only ask for credentials when the library makes the request to create a client. This way credentials will not be stored.

Discussion:
alejandrox1: The branch of which #49 was based off was updated before it was merged. Due to this, i am creating a new pr to resolve #48 

Patch:
diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 0000000..dc85065
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,19 @@
+---
+language: python
+
+python:
+  - "2.7"
+  - "3.3"
+  - "3.6"
+
+install:
+  - pip install -r requirements.txt
+
+script:
+  - py.test tests
+
+before_install:
+  - pip install sphinx>=1.6.3
+  - pip install pytest
+  - pip install mock
+  - python setup.py install
diff --git a/Makefile b/Makefile
index 5605c02..2735419 100644
--- a/Makefile
+++ b/Makefile
@@ -25,6 +25,14 @@ build: # Build development container.
 clean:
 	rm -rf agavepy.egg-info build dist .cache
 	rm -rf schema openapi
+	rm -rf agavepy/__pycache__/
+	rm -rf agavepy/tests/__pycache__/
+	rm -rf agavepy/*.pyc
+	rm -rf tests/__pycache__/
+	rm -rf tests/*.pyc
+	rm -rf .pytest_cache/
+
+clean-docs:
 	make -C docs/ clean
 
 deps:
@@ -32,19 +40,25 @@ deps:
 	mkdir -p openap
 
 docs: deps
-	python scripts/swagger_to_rst.py && \
-	cd docs && \
-	make static-clean && \
-	make openapi && \
-	make schema && \
-	make html && \
+	python scripts/swagger_to_rst.py
+	cd docs
+	make static-clean
+	make openapi
+	make schema
+	make html
 	cd ../
 
 install:
 	python setup.py install
 
+install-py2:
+	python2 setup.py install
+
 shell: build # Start a shell inside the build environment.
 	$(DOCKER_RUN_AGAVECLI) bash
 
 tests:
-	py.test agavepy/tests/test_agave_basic.py -s
+	pytest -v --cache-clear tests/
+
+tests-py2:
+	python2 -m pytest tests
diff --git a/README.rst b/README.rst
index 1441513..aed9336 100644
--- a/README.rst
+++ b/README.rst
@@ -114,23 +114,26 @@ documentation and your specific usage needs.
 
 Create a new Oauth client
 ^^^^^^^^^^^^^^^^^^^^^^^^^
+In order to interact with Agave, you'll need to first create an Oauth client so
+that later on you can create access tokens to do work.
+
+To create a client you can do the following:
 
 .. code-block:: pycon
 
-   >>> ag = Agave(api_server='https://api.tacc.cloud',
-   ...            username='mwvaughn',
-   ...            password='PaZ$w0r6!')
-   >>> ag.clients.create(body={'clientName': 'my_client'})
-   {u'consumerKey': u'kV4XLPhVBAv9RTf7a2QyBHhQAXca', u'_links': {u'subscriber':
-   {u'href': u'https://api.tacc.cloud/profiles/v2/mwvaughn'}, u'self': {u'href':
-    u'https://api.tacc.cloud/clients/v2/my_client'}, u'subscriptions': {u'href':
-    u'https://api.tacc.cloud/clients/v2/my_client/subscriptions/'}},
-    u'description': u'', u'tier': u'Unlimited', u'callbackUrl': u'',
-    u'consumerSecret': u'5EbjEOcyzzIsAAE3vBS7nspVqHQa', u'name': u'my_client'}
-
-You use the **consumerKey** and **consumerSecret** to generate Oauth *tokens*, 
+    >>> from agavepy.agave import Agave
+    >>> ag = Agave(api_server='https://api.tacc.cloud')
+    >>> ag.clients_create("client-name", "some description")
+    API username: your-username
+    API password: 
+    >>> ag.api_key
+    'xxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
+    >>> ag.api_secret
+    'XXXXXXXXXXXXXXXXXXXXXXXXXXXXX'
+
+You will use the api key and secret to generate Oauth *tokens*, 
 which are temporary credentials that you can use in place of putting your real 
-credentials into code that is scripting against the TACC APIs.
+credentials into code that is interacting with TACC APIs.
 
 Reuse an existing Oauth client
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -145,21 +148,13 @@ AgavePy up to use it works the same way:
 
    >>> from agavepy.agave import Agave
    >>> ag = Agave(api_server='https://api.tacc.cloud',
-   ...            username='mwvaughn', password='PaZ$w0r6!',
+   ...            username='mwvaughn',
    ...            client_name='my_client',
    ...            api_key='kV4XLPhVBAv9RTf7a2QyBHhQAXca',
    ...            api_secret='5EbjEOcyzzIsAAE3vBS7nspVqHQa')
 
 The Agave object ``ag`` is now configured to talk to all TACC Cloud services.
-Here's an example: Let's retrieve a the curent user's **profile**.
-
-.. code-block:: pycon
 
-   >>> ag.profiles.get()
-   {u'status': u'', u'username': u'mwvaughn', u'first_name': u'Matthew', 
-    u'last_name': u'Vaughn', u'phone': u'867-5309', u'mobile_phone': u'', 
-    u'create_time': u'20140515180317Z', u'full_name': u'vaughn', 
-    u'email': u'mwvaughn@devnull.com'}
 
 The refresh token
 ^^^^^^^^^^^^^^^^^
diff --git a/agavepy/agave.py b/agavepy/agave.py
index 8c26266..ef1c960 100644
--- a/agavepy/agave.py
+++ b/agavepy/agave.py
@@ -19,6 +19,7 @@ import jinja2
 import dateutil.parser
 import requests
 
+
 import sys
 sys.path.insert(0, os.path.dirname(__file__))
 
@@ -26,6 +27,7 @@ from .swaggerpy.client import SwaggerClient
 from .swaggerpy.http_client import SynchronousHttpClient
 from .swaggerpy.processors import SwaggerProcessor
 
+from .clients import client_create
 from .tenants import tenant_list
 
 HERE = os.path.dirname(os.path.abspath(__file__))
@@ -225,10 +227,15 @@ class Agave(object):
                 value = (kwargs[param] if mandatory
                          else kwargs.get(param, default))
             except KeyError:
+                # Request user to set the tenant url (api_server).
                 if param == "api_server":
                     self.list_tenants(tenantsurl="https://api.tacc.utexas.edu/tenants")
                     value = input(
                         "\nPlease specify the url of a tenant to interact with: ")
+                    
+                    # If present, remove the last '/' from the url.
+                    if value[-1] == "/":
+                        value = value[:-1]
 
                 else:
                     raise AgaveError("parameter \"{}\" is mandatory".format(param))
@@ -519,6 +526,29 @@ class Agave(object):
         """
         tenant_list(tenantsurl)
 
+    def clients_create(self, client_name, description):
+        """ Create an Agave Oauth client
+
+        Save the api key and secret upon a successfull reuest to Agave.
+
+        PARAMETERS
+        ----------
+        client_name: string
+            Name of the oauth client to be created.
+        description: string
+            Description of the client to be created.
+        """
+        # Set tenant url.
+        tenant_url = self.api_server
+        
+        # Set username.
+        if self.username == "" or self.username is None:
+            self.username = input("API username: ")
+
+        self.api_key, self.api_secret = client_create(
+            self.username, client_name, description, tenant_url)
+
+
 
 class Resource(object):
 
diff --git a/agavepy/clients.py b/agavepy/clients.py
new file mode 100644
index 0000000..fb3220a
--- /dev/null
+++ b/agavepy/clients.py
@@ -0,0 +1,82 @@
+"""
+    clients.py
+
+Functions related to Agave Oauth clients.
+"""
+from __future__ import print_function
+from builtins import input
+import getpass
+import json
+import requests
+import sys
+from os import path
+from utils import handle_bad_response_status_code
+
+
+class AgaveClientError(Exception):
+    """ Handle Agave-related client operations
+    """
+    def __init__(self, msg):
+        self.msg = msg
+
+    def __str__(self):
+        return repr(self.msg)
+
+
+def client_create(username, client_name, description, tenant_url):
+    """ Create an Agave client
+
+    Make a request to Agave to create an oauth client. Returns the client's api
+    key and secret as a tuple.
+    
+    PARAMETERS
+    ----------
+    username: string
+        User's username.
+    client_name: string
+        Name for agave client.
+    description: string
+        Description of the agave client.
+    tenant_url: string
+        URL of agave tenant to interact with.
+
+    RETURNS
+    -------
+    api_key: string
+    api_secret: string
+    """
+    # Get user's password.
+    passwd = getpass.getpass(prompt="API password: ")
+
+    # Set request endpoint.
+    endpoint = tenant_url + "/clients/v2"
+
+    # Make sure client_name is valid.
+    if client_name == "" or client_name is None:
+        raise AgaveClientError("Error creating client: invalid client_name")
+
+    # Make request.
+    try:
+        data = {
+            "clientName": client_name,
+            "description": description,
+            "tier": "Unlimited",
+            "callbackUrl": "",
+        }
+        resp = requests.post(endpoint, data=data, auth=(username, passwd))
+        del passwd
+    except Exception as err:
+        del passwd
+        raise AgaveClientError(err)
+
+    # Handle bad status code.
+    handle_bad_response_status_code(resp)
+
+    # Parse the request's response and return api key and secret.
+    response   = resp.json().get("result", {})
+    api_key    = response.get("consumerKey", "")
+    api_secret = response.get("consumerSecret", "")
+    if api_key == "" or api_secret == "":
+        raise AgaveClientError("Error creating client: api key and secret are empty")
+
+    return api_key, api_secret
diff --git a/agavepy/tenants.py b/agavepy/tenants.py
index a3dc70d..85a0222 100644
--- a/agavepy/tenants.py
+++ b/agavepy/tenants.py
@@ -6,7 +6,7 @@ Methods to interact with Agave tenants.
 from __future__ import print_function
 import requests
 import sys
-from .response_handlers import handle_bad_response_status_code
+from utils import handle_bad_response_status_code
 
 
 def get_tenants(url):
diff --git a/agavepy/utils/__init__.py b/agavepy/utils/__init__.py
new file mode 100644
index 0000000..fb93af6
--- /dev/null
+++ b/agavepy/utils/__init__.py
@@ -0,0 +1,1 @@
+from .response_handlers import handle_bad_response_status_code
diff --git a/agavepy/utils/response_handlers.py b/agavepy/utils/response_handlers.py
new file mode 100644
index 0000000..e98a8dc
--- /dev/null
+++ b/agavepy/utils/response_handlers.py
@@ -0,0 +1,28 @@
+"""
+    response_hanlders.py
+"""
+from __future__ import print_function
+import sys
+
+
+
+class AgaveAPICallError(Exception):
+    """ Handle bad responses from Agave
+    """
+    def __init__(self, msg):
+        self.msg = msg
+
+    def __str__(self):
+        return repr(self.msg)
+
+
+def handle_bad_response_status_code(r):                                         
+    """ Handle a response with a bad status code                                
+    """
+    if not r.ok:
+        error_msg = "Bad {0} request to {1}, status code {2}\n".format(
+            r.request.method, r.url, r.status_code),                        
+        error_msg += "{}\n".format(r.request.body)
+        error_msg += "{}\n".format(r.json())
+
+        raise AgaveAPICallError(error_msg)
diff --git a/dev.Dockerfile b/dev.Dockerfile
index 3bc256f..a982d1b 100644
--- a/dev.Dockerfile
+++ b/dev.Dockerfile
@@ -16,7 +16,10 @@ RUN apt-get update -y && apt-get install -yq git bash-completion \
     && make altinstall \
     && ln -sf /usr/local/bin/python2.7 /usr/bin/python2 \
     && cd ../ \
-    && rm -r Python-${PYVERSION}.tgz Python-${PYVERSION}
+    && rm -r Python-${PYVERSION}.tgz Python-${PYVERSION} \
+    && pip install sphinx \
+    && pip install pytest \
+    && pip2.7 install mock
 
 RUN git clone https://github.com/TACC/agavepy 
 


Test patch:
diff --git a/TESTING.md b/TESTING.md
new file mode 100644
index 0000000..79b2575
--- /dev/null
+++ b/TESTING.md
@@ -0,0 +1,58 @@
+# Testing
+
+## Writing new tests
+
+Most code changes will fall into one of the following categories.
+
+### Writing tests for new features
+
+New code should be covered by unit tests. If the code is difficult to test with
+a unit tests then that is a good sign that it should be refactored to make it
+easier to reuse and maintain. Consider accepting unexported interfaces instead
+of structs so that fakes can be provided for dependencies.
+
+
+### Writing tests for bug fixes
+
+Bugs fixes should include a unit test case which exercises the bug.
+
+
+### Test structure
+
+Test serve a dual purpose: test functionality and document the expected
+response from the agave API.
+
+The test suite uses valid sample responses, see 
+[sample responses](tests/sample_responses) to mock the agave API by spawning a
+mock server in the local host to handle any requests from the cli.
+
+
+## Running tests
+
+To run the unit test suite run the development container:
+```
+$ make shell
+```
+
+### Testing Python 3
+Before you start testing, you'll need to install all the package dependencies.
+```
+$ make install
+```
+
+Inside the container run:
+```
+$ make tests
+```
+
+
+### Testing Python 2
+Install all dependencies
+```
+$ make install-p2
+```
+
+Run tests
+```
+$ make tests-py2
+```
diff --git a/tests/clients_test.py b/tests/clients_test.py
new file mode 100644
index 0000000..be061b1
--- /dev/null
+++ b/tests/clients_test.py
@@ -0,0 +1,112 @@
+"""
+    client_tests.py
+
+Test operations related to the management of Agave oauth clients.    
+"""
+import pytest
+import cgi
+import json
+import os
+import sys
+from http.server import BaseHTTPRequestHandler
+from testsuite_utils import MockServer
+from response_templates import response_template_to_json
+
+try:
+    from unittest.mock import patch
+except ImportError:
+    from mock import patch
+
+sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
+from agavepy.agave import Agave
+
+
+# The agave API will provide a response with the following format upon a
+# successfull attempt at creating a client.
+sample_client_create_response = response_template_to_json("clients-create.json")
+
+
+class MockServerClientEndpoints(BaseHTTPRequestHandler):
+    """ Mock the Agave API
+
+    Mock client managament endpoints from the agave api.
+    """
+
+    def do_POST(self):
+        """ Test agave client creation
+        """
+        # Get request data.
+        form = cgi.FieldStorage(
+            fp = self.rfile,
+            headers = self.headers,
+            environ={'REQUEST_METHOD': 'POST'})
+
+        # Check client name is set.
+        client_name = form.getvalue("clientName", "")
+        if client_name == "":
+            self.send_response(400)
+            self.end_headers()
+            return
+
+        # Check client description is set.
+        client_description = form.getvalue("description", "")
+        if client_description == "":
+            self.send_response(400)
+            self.end_headers()
+            return
+
+        # CHeck client tier is set.
+        if form.getvalue("tier", "") == "":
+            self.send_response(400)
+            self.end_headers()
+            return
+
+        # Update response fields.
+        sample_client_create_response["result"]["name"] = client_name
+        sample_client_create_response["result"]["description"] = client_description
+        sample_client_create_response["result"]["consumerKey"] = "some api key"
+        sample_client_create_response["result"]["consumerSecret"] = "some secret"
+
+        self.send_response(200)
+        self.end_headers()
+        self.wfile.write(json.dumps(sample_client_create_response).encode())
+
+
+
+class TestMockServer(MockServer):
+    """ Test client-related agave api endpoints 
+
+    Tests client creation (HTTP POST request), removal (HTTP DELETE request), 
+    and listing (HTTP GET request).
+    """
+
+    @classmethod
+    def setup_class(cls):
+        """ Set up an agave mock server
+
+        Listen and serve mock api as a daemon.
+        """
+        MockServer.serve.__func__(cls, MockServerClientEndpoints)
+
+
+    @patch("agavepy.agave.input")
+    @patch("clients.getpass.getpass")
+    def test_client_create(self, mock_input, mock_pass):
+        """ Test client create op
+
+        Patch username and password from user to send a client create request 
+        to mock server.
+        """
+        # Patch username and password.
+        mock_input.return_value = "user"
+        mock_pass.return_value = "pass"
+
+        # Instantiate Agave object making reference to local mock server.
+        local_uri = "http://localhost:{port}/".format(port=self.mock_server_port)
+        ag = Agave(api_server=local_uri)
+
+        # Create client.
+        ag.clients_create("client-name", "some description")
+
+        assert ag.api_key == "some api key"
+        assert ag.api_secret == "some secret"
diff --git a/tests/response_templates.py b/tests/response_templates.py
new file mode 100644
index 0000000..18b8356
--- /dev/null
+++ b/tests/response_templates.py
@@ -0,0 +1,14 @@
+import json
+import pkg_resources
+
+
+rsc_pkg = __name__
+
+
+def response_template_to_json(template_name):
+    rsc_path = "/".join(("sample_responses", template_name))
+    try: # Python 3.3 will return the contents as a bytes-like object.
+        file_contents = pkg_resources.resource_string(rsc_pkg, rsc_path).decode("utf-8")
+    except AttributeError:
+        file_contents = pkg_resources.resource_string(rsc_pkg, rsc_path)
+    return json.loads(file_contents)
diff --git a/tests/sample_responses/clients-create.json b/tests/sample_responses/clients-create.json
new file mode 100644
index 0000000..a3e6789
--- /dev/null
+++ b/tests/sample_responses/clients-create.json
@@ -0,0 +1,24 @@
+{                                                     
+    "status": "success",                                                        
+    "message": "Client created successfully.",                                  
+    "version": "2.0.0-SNAPSHOT-rc3fad",                                         
+    "result": {                                                                 
+        "description": "{DESCRIPTION}",                                         
+        "name": "{NAME}",                                                       
+        "consumerKey": "xxxxxxxxxxxxxxxxxxxxxxxxxx",                            
+        "_links": {                                                             
+            "subscriber": {                                                     
+                "href": "{URL_ROOT}profiles/v2/{USER}"                          
+            },                                                                  
+            "self": {                                                           
+                "href": "{URL_ROOT}clients/v2/{NAME}"                           
+            },                                                                  
+            "subscriptions": {                                                  
+                "href": "{URL_ROOT}clients/v2/{NAME}/subscriptions/"            
+            }                                                                   
+        },                                                                      
+        "tier": "{TIER}",                                                       
+        "consumerSecret": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",                  
+        "callbackUrl": "{CALLBACK_URL}"                                         
+    }                                                                           
+}
diff --git a/tests/testsuite_utils.py b/tests/testsuite_utils.py
new file mode 100644
index 0000000..d77644f
--- /dev/null
+++ b/tests/testsuite_utils.py
@@ -0,0 +1,50 @@
+"""
+    testsuite_utils.py
+
+Methods used throughout the test suite for testing.
+"""
+import socket
+from threading import Thread
+try: # python 2
+    from BaseHTTPServer import HTTPServer
+except ImportError: # python 3
+    from http.server import HTTPServer
+
+
+
+def get_free_port():
+    """ Find a free port
+
+    Return a port available for connecting on localhost.
+    """
+    s = socket.socket(socket.AF_INET, type=socket.SOCK_STREAM)
+    s.bind(("localhost", 0))
+    addr, port = s.getsockname()
+    s.close()
+    return port
+
+
+class MockServer(object):
+    """ Mock server
+
+    Run an HTTP server as a daemon in a thread.
+    """
+
+    @classmethod
+    def serve(cls, http_server):
+        """ Set up mock server
+
+        INPUTS
+        -------
+        http_server: BaseHTTPRequestHandler
+            HTTP server with request handlers specified
+        """
+        # Find a port to listen to connect.
+        cls.mock_server_port = get_free_port()
+        # Instantiate server.
+        cls.mock_server = \
+                HTTPServer(("localhost", cls.mock_server_port), http_server)
+
+        cls.mock_server_thread = Thread(target=cls.mock_server.serve_forever)
+        cls.mock_server_thread.setDaemon(True)
+        cls.mock_server_thread.start()