Support IAM authentication in replication documents #365
Conversation
|
Update copyright for |
src/cloudant/_client_session.py
Outdated
| 'IAM_TOKEN_URL', 'https://iam.bluemix.net/identity/token') | ||
|
|
||
| @property | ||
| def api_key(self): |
There was a problem hiding this comment.
Should this be named get_api_key following the design of the functions in this class e.g. _get_access_token?
| type(m_admin_party_client).admin_party = mock.PropertyMock( | ||
| return_value=True) | ||
| type(m_admin_party_client).server_url = mock.PropertyMock( | ||
| return_value=self.server_url) |
There was a problem hiding this comment.
I'm wondering if we should put this code into it's own method so that it can be called by all tests in this class. It would require 4 if blocks for options admin_party, server_url, r_session, and is_iam_authenticated.
ricellis
left a comment
ricellis
left a comment
There was a problem hiding this comment.
I've not reviewed the tests for now as I think this might need bigger changes. I'm reluctant for it to scope creep into fixing the old source/target URL issue, but I think that is a lot easier if we remove the access to the current credentials and force them to be supplied appropriately for creating replicator documents.
| create the target, if it does not already exist. | ||
| :param bool continuous: If set to True then the replication will be | ||
| continuous. | ||
There was a problem hiding this comment.
The existing user_ctx applies to a target only. I think we need to make source_user(_ctx?) and target_user(_ctx?) args that can be used to specify. We should alias the user_ctx to target_user to maintain backwards compatibility.
There was a problem hiding this comment.
Not sure I understand what you mean here. Can we please go through it when you get a mo? Thanks.
There was a problem hiding this comment.
It was a long time ago and I'm struggling to reverse engineer my meaning too! I think what I was getting at was that the user_ctx default setting is dependent on some checks on the target, but not the source. My suggestion above doesn't make sense either though.
What I think makes sense is that the default user_ctx is set if one isn't supplied, but should not depend on any checks for the source or target database.
There was a problem hiding this comment.
Done. Removed admin party check in eaaa7f1.
| ) | ||
| if source_db.admin_party: | ||
| pass # no credentials required | ||
| elif source_db.client.is_iam_authenticated: |
There was a problem hiding this comment.
The auth used in a replicator document should be independent of the method with which the client is authenticated. For example there is no reason why I can't login python-cloudant with my account credentials and create a replicator document using any other credentials including IAM API keys.
This code should be a fallback only if the source_user is empty and target_user are empty. I think however, that a new version of the create_replication should require some credential information and the fallback should only be used in the backwards compatibility case.
There was a problem hiding this comment.
source_db and target_db are supplied to the create_replication method. They are type database objects and therefore don't share credentials used to POST the replication doc.
There was a problem hiding this comment.
Yes, I see what you mean, everything is an object, so the specific credentials will have been supplied up front in construction of the necessary source or target database objects (or more specifically the client instances needed for those if they are distinct from the replicator.
I think it would be nice if a replication could be created with a source & target URLs and the associated API keys i.e. 4 strings instead of needing to potentially instantiate multiple clients and DBs, but that is out of scope here.
There was a problem hiding this comment.
Just want to note that we made a decision to remove source/target URLs from replication (#342) as it wasn't working properly.
There was a problem hiding this comment.
Yep, that's why I think it is out of scope to try and re-introduce that in a working fashion here.
src/cloudant/replicator.py
Outdated
| data['target'].update( | ||
| {'headers': {'Authorization': target_db.creds['basic_auth']}} | ||
| ) | ||
| if source_db.admin_party: |
There was a problem hiding this comment.
Shouldn't this be the target? Also shouldn't we only do these checks if a user_ctx wasn't provided?
smithsz
force-pushed
the
354-support-iam-auth-in-replications
branch
8 times, most recently
from
eaaa7f1 to
04deb46
Compare
| pass # noop - not required for admin party mode | ||
| elif self.database.creds and self.database.creds.get('user_ctx'): | ||
| data['user_ctx'] = self.database.creds['user_ctx'] | ||
| if not data.get('user_ctx') and self.database.creds and \ |
There was a problem hiding this comment.
Is there a case where user_ctx doesn't exist in data and database.creds?
There was a problem hiding this comment.
yeah, admin party mode.
smithsz
force-pushed
the
354-support-iam-auth-in-replications
branch
from
04deb46 to
8cbee2a
Compare
3 participants
Thanks for your hard work, please ensure all items are complete before opening.
What
Support IAM authenticated databases when replicating.
How
Expose the IAM API key via the client session and inject into the replication document where applicable.
Testing
Includes additional mock unit tests.
Issues
Fixes #354.