303 support iam auth #306

smithsz · 2017-06-28T16:49:39Z

What

Add IAM authentication support.

How

Added a class method and context manager to simplify IAM client construction:

from cloudant.client import Cloudant
client = Cloudant.iam('account_name', 'api_key_here', connect=True)
print client.all_dbs()

# or using context manager...
from cloudant import cloudant_iam
with cloudant_iam('account_name', 'api_key_here') as client:
    print client.all_dbs()

Under the hood there's a new use_iam=True toggle. Once enabled, the password parameter is used as the IAM API key.

IAM Authentication Workflow:

Exchange IAM API key for access token via IAM_TOKEN_URL.
Generate a cookie from the access token using the POST /_iam_session Cloudant endpoint.
Use the IAMSession cookie in all library requests.
Attempt to renew if the IAMSession cookie has expired or on any 401 response.

Testing

Includes additional mock tests.

Issues

Fixes #303.

emlaver · 2017-06-28T17:05:57Z

src/cloudant/__init__.py

+    Provides a context manager to create a Cloudant session and provide access
+    to databases, docs etc.
+
+    :param api_key: IAM authentication API key.


Can we add an example similar to what we have below in cloudant_bluemix?

Yep, will do. I'll improve the getting started stuff too once I find the proper IAM blumix docs link.

ricellis

A few thoughts.

ricellis · 2017-07-05T14:37:45Z

tests/unit/iam_auth_tests.py

+
+    @mock.patch('cloudant._common_util.IAMSession.login')
+    @mock.patch('cloudant._common_util.ClientSession.request')
+    def test_iam_renew_cookie_on_403(self, m_req, m_login):


This test is invalid. A 403 should not trigger a renew.

ricellis · 2017-07-05T14:39:41Z

src/cloudant/_common_util.py

+            return resp
+
+        is_expired = any((
+            resp.status_code == 403 and


_iam_session shouldn't have this same 403 quirk as Cloudant's _session.

ricellis · 2017-07-05T15:20:40Z

src/cloudant/_common_util.py


-class InfiniteSession(Session):
+
+class ClientSession(Session):


The majority of the work that CookieSession and IAMSession have to do is the same, other than the extra piece to get an IAM token (and the content-type of the POST to *_session which should be application/json for IAM).
It would be nice (but by no means essential if it is harder than it initially looks) if we could use the same login/logout/expired checks in this base class and just customize them slightly (e.g. with the url/data/response conditions).

It's a little tricky as ClientSession is used directly when admin party is enabled.
I still think it's worth doing. I've raise issue #310 to track the work.

ricellis · 2017-07-05T15:24:31Z

docs/getting_started.rst

+    client = Cloudant(USERNAME, API_KEY, account=ACCOUNT_NAME,
+                     auto_renew=True,
+                     connect=True,
+                     use_iam=True)


I'm not a fan of exposing yet another parameter on here, I think it is particularly confusing that the USERNAME is effectively ignored in this scenario. I wonder whether we can have another way of initializing that effectively hides this complexity.

Added new class method in 9f181e7.

Can we safely omit this property from the docs then?

Ah, forgot to update docs. Fixed in cd1c664.

ricellis · 2017-07-05T15:26:49Z

tests/unit/iam_auth_tests.py

+    @mock.patch('cloudant._common_util.ClientSession.request')
+    def test_iam_get_access_token(self, m_req):
+        m_response = mock.MagicMock()
+        m_response.json.return_value = {'access_token': 'bar'}


This mock response should probably have more of the expected token content in it.

ricellis · 2017-07-25T13:56:37Z

docs/getting_started.rst

+    client = Cloudant(USERNAME, API_KEY, account=ACCOUNT_NAME,
+                     auto_renew=True,
+                     connect=True,
+                     use_iam=True)


Can we safely omit this property from the docs then?

ricellis · 2017-07-25T13:59:48Z

src/cloudant/client.py

        `Requests library timeout argument
        <http://docs.python-requests.org/en/master/user/quickstart/#timeouts>`_.
        but will apply to every request made using this client.
+    :param bool use_iam: Keyword argument, if set to True performs


Should add a line here expressing that the preferred approach is to use the iam function.

Added in 9d0a5f3.

ricellis · 2017-07-25T14:16:21Z

tests/unit/iam_auth_tests.py

+
+        self.assertEqual(info, m_info)
+        self.assertTrue(m_response.raise_for_status.called)
+


I feel like there is a golden path test missing here, checking that a request is as expected without any expiry/renewal conditions.

I also think it would be valuable to have a test at the client level verifying that using the class method creates the IAM session and appropriate golden path is followed making a normal request (token, iam_session, normal request).

Added in 85932a9 & fc8d64a.

mikerhodes · 2017-07-28T12:07:36Z

src/cloudant/_common_util.py

+        self._auto_renew = kwargs.get('auto_renew', False)
+        self._session_url = url_join(server_url, '_iam_session')
+        self._token_url = os.environ.get(
+            'IAM_TOKEN_URL', 'https://iam.bluemix.net/oidc/token')


Can this be made a kwarg rather than an env var? Why is it an env var?

Our view was that it should basically never be necessary to change the token URL, but we thought there was value in providing an "override" primarily for the event that for some reason the expected URL was unavailable (although it also has value in testing).

I think an env var is more suitable for this purpose in that it doesn't require a code change if, for example, one needed to switch token server to a backup to temporarily workaround some kind of outage. That was the thinking anyway.

mikerhodes · 2017-07-28T12:07:56Z

docs/getting_started.rst

+.. code-block:: python
+
+    # Authenticate using an IAM API key
+    client = Cloudant.iam(ACCOUNT_NAME, API_KEY, connect=True)


Do you want to mention here the environment variable that allows for different IAM servers? (Note I query below whether it should be an env var rather than a kwarg).

Added in 3b4edd9.

mikerhodes · 2017-07-28T12:33:54Z

src/cloudant/_common_util.py

-            method, url, timeout=self._timeout, **kwargs)
+        self.cookies.clear_expired_cookies()
+        if self._auto_renew and 'IAMSession' not in self.cookies.keys():
+            self.login()


I'd rather this be more explicitly about the expiry of the IAMSession cookie, it took me a bit to understand that's what was actually happening. Like:

if self._auto_renew and 'IAMSession' not in self.cookies.keys() and not self.cookies['IAMSession'].is_expired(): self.login()

You can't get a cookie object from the jar without iterating over the entire jar. self.cookies['IAMSession'] just gives your the cookie value as a string.
We'd need something horrible like:

self.cookies._cookies['mikerhodes-iam-testing.cloudant.com']['/']['IAMSession'].is_expired()

I think sticking to self.cookies.clear_expired_cookies() is the cleanest solution.

I guess add a comment to the effect of this being the simplest way to check whether we have an expired cookie so the next person doesn't have to figure it out by guesswork. A decent "why I'm doing it" comment rather than a "describe what the code does" comment.

mikerhodes

Approved, though I'd like to see the comment around the mess of an API the cookie jar has.

These checks are redundant. Session endpoint requests are always made using the base request method.

emlaver reviewed Jun 28, 2017

View reviewed changes

ricellis self-requested a review July 5, 2017 14:09

ricellis reviewed Jul 5, 2017

View reviewed changes

smithsz force-pushed the 303-support-iam-auth branch from dc88595 to f0cc64e Compare July 23, 2017 09:44

ricellis suggested changes Jul 25, 2017

View reviewed changes

ricellis approved these changes Jul 26, 2017

View reviewed changes

smithsz force-pushed the 303-support-iam-auth branch from 964d62e to 92be4f4 Compare July 27, 2017 07:55

mikerhodes suggested changes Jul 28, 2017

View reviewed changes

mikerhodes approved these changes Aug 7, 2017

View reviewed changes

ricellis mentioned this pull request Aug 8, 2017

Implement Cloudant.bluemix client class method #314

Merged

4 tasks

ricellis modified the milestone: 2.next Aug 8, 2017

smithsz added 6 commits August 29, 2017 12:46

Add IAM authentication support

090afb7

Rename InfiniteSession -> CookieSession in unit tests

cdba134

Add IAM authentication tests

c828b85

Add IAM notes to getting_started.rst

0f6176b

Allow multiple calls to client .connect()

275057a

Renew IAM token on 401 status code or cookie expiry

59ca68f

smithsz force-pushed the 303-support-iam-auth branch from 2ca785b to fcbe630 Compare August 29, 2017 11:48

smithsz added 7 commits August 29, 2017 12:48

Add IAM class method to Cloudant class

6b85dc3

Always use auto_renew=True by default for IAM sessions

c115f9c

Remove session endpoint checks

54d8fbe

These checks are redundant. Session endpoint requests are always made using the base request method.

Remove unused CouchDB._client_session

5f4935a

Update year in copyright headers

550b00b

Add IAM_TOKEN_URL env var note to docs/getting_started.rst

fdb08de

Add code comment about discarding expired IAMSession cookies

fcbe630

smithsz force-pushed the 303-support-iam-auth branch from 1d25ed5 to 2dd2b52 Compare August 29, 2017 16:14

Allow CouchDB.session_login to take credentials as arguments

1a905fb

smithsz force-pushed the 303-support-iam-auth branch from 2dd2b52 to 1a905fb Compare August 29, 2017 16:40

smithsz merged commit 73cb8b1 into master Sep 5, 2017

smithsz deleted the 303-support-iam-auth branch September 5, 2017 10:14


		class InfiniteSession(Session):

		class ClientSession(Session):


		self.assertEqual(info, m_info)
		self.assertTrue(m_response.raise_for_status.called)

303 support iam auth #306

303 support iam auth #306

Uh oh!

Conversation

smithsz commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Testing

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricellis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikerhodes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smithsz commented Jun 28, 2017 •

edited

Loading