Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth using authsource db #207

Merged

Conversation

harlov
Copy link

@harlov harlov commented Apr 26, 2017

Use 'authsource' as auth database, if specify in uri, and orig database if not.

https://docs.mongodb.com/manual/reference/connection-string/#urioption.authSource

@coveralls
Copy link

coveralls commented Apr 26, 2017

Coverage Status

Coverage increased (+0.005%) to 95.194% when pulling 5d22504 on harlov:AuthSource_ignored_on_connect into 80c65aa on twisted:master.

Copy link
Contributor

@IlyaSkriblovsky IlyaSkriblovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a test for this new supported uri option? Although code change is pretty obvious, this unit test could help us not to loose this functionality in future. You can probably just clone test_AuthConnectionPoolUri test method in tests/test_auth.py and change it accordingly.

The record in docs/source/NEWS.rst would be appreciated too!

@@ -298,6 +298,10 @@ def __tcp_or_ssl_connect(self, host, port, factory, **kwargs):
return reactor.connectTCP(host, port, factory, **kwargs)

@property
def auth_db(self):
return self.__uri['options'].get('authsource') or self.__uri['database']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property adds to the public API of Connection without obvious reason. Why do we need this as a public property? Or even if we need it, its name falsely hints that it will return Database object, not str. My thinking that there is no bold reason to make this public and it is better to make it local variable inside __init__.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, property reduntant. I will fix it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -271,8 +271,8 @@ def __init__(self, uri="mongodb://127.0.0.1:27017", pool_size=1, ssl_context_fac
for i in range(pool_size)
]

if self.__uri['database'] and self.__uri['username'] and self.__uri['password']:
self.authenticate(self.__uri['database'], self.__uri['username'],
if self.__uri['database'] and self.__uri['username'] and self.__uri['password']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for tediousness, but could you please remove the trailing whitespace?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry) i will fix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@IlyaSkriblovsky
Copy link
Contributor

Thanks for your contribution! Could you please address a couple of comments above?

@harlov harlov force-pushed the AuthSource_ignored_on_connect branch from 5d22504 to 2ff44f0 Compare April 26, 2017 14:38
@coveralls
Copy link

coveralls commented Apr 26, 2017

Coverage Status

Coverage increased (+0.003%) to 95.192% when pulling 2ff44f0 on harlov:AuthSource_ignored_on_connect into 80c65aa on twisted:master.

@IlyaSkriblovsky
Copy link
Contributor

Thank you!

@IlyaSkriblovsky IlyaSkriblovsky merged commit 1c9e701 into twisted:master Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants