let username and password be specified outside a uri #731

ThisGuyCodes · 2014-08-11T19:09:20Z

currently, if you use a uri without the username/password/db name inside it, ie:
mongodb://host1.com:1337,host2.com:1337/
it will pull the db name from the other args passed to connet():
'name': uri_dict.get('database') or name,
but the username and password specified in connect() will be overwritten with None:

'password': uri_dict.get('password'), # <---Returns None if no password was in the uri
'password': uri_dict.get('password'),

This forces anyone using a uri, say to make handling multiple nodes easier, to put their username and password into the uri. This isn't a huge problem, but forces some awkward manual string formatting in some setups, and there's no need for that.

I also removed a duplicate read_preference update.

since parse_uri() returns None for values not specified in the uri, 'name' was allowed to be specified outside the uri. This commit allows the same for username and password.

read_preference was already put in the conn_settings dict earlier, and the version placed here is unchanged, remove it.

yograterol · 2014-08-11T19:59:57Z

@conslo good, can you add a test case please for that case?

yograterol · 2014-08-20T19:41:40Z

ping @conslo

ThisGuyCodes · 2014-08-20T19:43:43Z

@yograterol sorry, been busy, I should have this done later today or tomorrow

This setting was being removed from the dict it’s being compared to, but not it. This would cause any connection_setting with an authentication_source set to always cause new connections to be made.

Explanation of that slightly cryptic loop through settings when establishing a connection

coveralls · 2014-08-28T20:10:23Z

Coverage increased (+0.04%) when pulling 8b0ae25 on conslo:master into bce7ca7 on MongoEngine:master.

ThisGuyCodes · 2014-08-28T20:27:21Z

mongoengine/connection.py

@@ -54,9 +54,8 @@ def register_connection(alias, name=None, host=None, port=None,
        uri_dict = uri_parser.parse_uri(conn_settings['host'])
        conn_settings.update({
            'name': uri_dict.get('database') or name,
-            'username': uri_dict.get('username'),
-            'password': uri_dict.get('password'),
-            'read_preference': read_preference,


this was redundant, as it gets overriden later anyway

ThisGuyCodes · 2014-08-28T20:30:55Z

tests/test_connection.py

+        # work fine, but there would be no username/password on the
+        # connection.
+        self.assertEqual(me_connection._connection_settings['test_uri_no_username']['username'], 'username')
+        self.assertEqual(me_connection._connection_settings['test_uri_no_username']['password'], 'password')


I really don't like doing it this way. If anyone knows how to check the authenticated username on a connection (or anything else that solves this problem) please let me know?

ThisGuyCodes · 2014-08-28T20:33:21Z

I also added some comments to explain some stuff in get_connection(). I'm at least 95% sure that I'm correct in my assumption, but it could use a look by someone more experienced with the project.

coveralls · 2014-08-28T22:48:11Z

Coverage increased (+0.04%) when pulling 974faad on conslo:master into bce7ca7 on MongoEngine:master.

coveralls · 2014-08-29T03:38:15Z

Coverage increased (+0.04%) when pulling 974faad on conslo:master into bce7ca7 on MongoEngine:master.

coveralls · 2014-08-29T04:54:01Z

Coverage increased (+0.04%) when pulling 974faad on conslo:master into bce7ca7 on MongoEngine:master.

Conflicts: mongoengine/connection.py

ThisGuyCodes · 2014-09-02T19:19:58Z

mongoengine/connection.py

                connection_settings.pop('name', None)
                connection_settings.pop('username', None)
                connection_settings.pop('password', None)
-                if conn_settings == connection_settings and _connections.get(db_alias, None):
-                    connection = _connections[db_alias]
+                connection_settings.pop('authentication_source', None)


comment got removed due to merge, but I feel it's worth being visible:
this was popped off the setting we were comparing, but not within the loop. This would cause any settings with an authentication_source set to equate as not equal every time.

coveralls · 2014-09-02T19:26:09Z

Coverage increased (+0.04%) when pulling bbcb97f on conslo:master into 6942f9c on MongoEngine:master.

MRigal · 2014-12-10T17:08:00Z

@conslo have you seen that the tests are not passing ? You should maybe explain the count difference.

Also, don't you think that the possibility to connect without adding username and password could introduce a security hole ?

ThisGuyCodes · 2014-12-10T19:13:50Z

Sorry, this totally fell off my radar. I haven't looked into it but at first glance it looks like there's a merge conflict, I'll reserve some cycles this week.

As for the security hole; I don't understand what you mean. It's up to the database to require or not require a username/password, not the connection library (you can have a mongo connection without them, in fact this is the default). What I'm doing here is allowing you to use a URI string but specify the username and password outside the URI.

MRigal · 2014-12-10T20:50:34Z

Right, I misunderstood the purpose. But then I have difficulties to see the improvement, especially if one anyway has to "initiate" the connection with username and password a first time.

ThisGuyCodes · 2014-12-10T21:59:21Z

I'll outline the case at my work where this came up to illustrate the reasoning.
We do not commit secrets to our repository, instead we have a setup like:

__init__.py
local.py
production.py

local.py is untracked in git, and contains lines like:
MONGO_USERNAME = "xxx"

production.py is tracked in git, and contains lines like:
MONGO_USERNAME = os.environ.get('MONGO_USERNAME', None)

Then __init__.py looks like this:

from .production import *
try:
    from .local import *
except ImportError:
    pass

Doing this allows us to put configuration in the environment on production, but still keep development values locally without constant re-setup.

In order to specify multiple databases (we have two: a master and slave, with auto-failover) you need to use the URI, ie:
MONGO_URI = "mongodb://host1.com:1337,host2.com:1337/"
But this won't work right now, as register_connection('foo', host=MONGO_URI, username=USERNAME, password=PASSWORD) with this URI will attempt a connection without a username and password. This is because if the host kwarg is a URI, it over-writes the username and password with the username and password pulled from the URI (which in this case are both None as I didn't put a username or password in the URI).

(I'm going to take this too far now, but as I was writing this it started getting humorous so I kept going)
I could put the username and password in the URI; but this is annoying to deal with. Imagine for instance you have n connections with different permissions (username + password combinations), but all use the same database. I also could do everything MongoEngine does manually with PyMongo, and I could do everything PyMongo does by manually reading and writing to/from a TCP socket. And I could do everything Python does with BF (or gods forbid, C). Just because there's already a way to get it done (we call these 'work arounds'), doesn't mean it's not something that should be fixed.

MRigal · 2014-12-11T12:42:09Z

@conslo ok I've understood the case and it makes sense. Just aside, I think that using a replica set and connecting via mongos (and therefore simplifying the connection parameters) could be more adapted to your setup, but that is out of scope ;-)

Anyway to get a chance to get the pull request merged, you should fix the tests not passing. I don't know if you noticed that 'test_multiple_connection_settings' is not passing anymore.

In this case, it is maybe really expected to get only one connection anymore with your fix. But then you should change this test and IMHO also write a new one with really different settings and an assert that we do have 2 distinct connections.

ThisGuyCodes · 2014-12-11T16:44:18Z

@MRigal could you elaborate on 'connecting via mongos'? I'm not totally fluent on mongo connection handling, but from what a quick set of googles tell me using this multi-host connection URI is using a replica set: doc. Save for this ?replicaSet=xxx option we're not using. If I understand correctly this option allows the driver to pull the location of the other instances from the DB instead of having them all handed to it? If this is the case that seems wonderful: but the documentation does still say When connecting to a replica set it is important to give a seed list of at least two mongod instances.

ThisGuyCodes · 2014-12-11T16:45:09Z

and yes I see that, I'll have to dig into it to understand if this is a failure or a change in behavior and change things accordingly.

MRigal · 2015-04-29T20:05:41Z

@conslo ping for the failed tests. I think the ground idea is quite good, but I also think the current logic is not completely right.
Since we have release 0.9 and we will release a new version soon, it may be a good time to work on it. Also a rebase would be good.

Regarding the connection credentials, what I was meaning is that the way MongoDB recommends to have the servers and the connections, for optimised security is a bit different from what you are using.
Instead of a master/slave, it should be a replica set (http://docs.mongodb.org/manual/core/master-slave/#convert-master-slave-to-replica-set), with at least an arbiter on another machine, and then, with a mongos instance on each machine you are using to connect to, you would not have these problems anymore of having to define a list of servers, since you connect to a replica set, which knows itself about the composing servers.

wojcikstefan · 2016-12-04T02:14:35Z

See #1421

ThisGuyCodes added 2 commits August 11, 2014 14:59

let username/password to be specified outside URI

dec6cf3

since parse_uri() returns None for values not specified in the uri, 'name' was allowed to be specified outside the uri. This commit allows the same for username and password.

remove duplicate read+preference line

aefa0a6

read_preference was already put in the conn_settings dict earlier, and the version placed here is unchanged, remove it.

ThisGuyCodes added 6 commits August 28, 2014 12:00

Merge branch 'parent-master'

4a63d35

match other connection setting pop

daa3fbe

This setting was being removed from the dict it’s being compared to, but not it. This would cause any connection_setting with an authentication_source set to always cause new connections to be made.

some comments

5081383

MOAR COMMENTS

8b0ae25

Explanation of that slightly cryptic loop through settings when establishing a connection

test for username/password outside a uri

122e58b

I'm not testing that here

b299a63

ThisGuyCodes added 2 commits August 28, 2014 16:25

expose some internals to actually test

8fc39f3

explanation

c601196

ThisGuyCodes reviewed Aug 28, 2014
View reviewed changes

finish incomplete explanation

974faad

ThisGuyCodes reviewed Aug 28, 2014
View reviewed changes

Merge branch 'parent-master'

bbcb97f

Conflicts: mongoengine/connection.py

ThisGuyCodes reviewed Sep 2, 2014
View reviewed changes

MRigal added the Awaiting Response label Apr 29, 2015

MRigal mentioned this pull request Apr 29, 2015

Connect blindly overrides username/password when URI style connection is used. #909

Closed

wojcikstefan mentioned this pull request Dec 4, 2016

Don't let the MongoDB URI override connection settings it doesnt explicitly specify #1421

Merged

wojcikstefan closed this Dec 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

let username and password be specified outside a uri #731

let username and password be specified outside a uri #731

ThisGuyCodes commented Aug 11, 2014

yograterol commented Aug 11, 2014

yograterol commented Aug 20, 2014

ThisGuyCodes commented Aug 20, 2014

coveralls commented Aug 28, 2014

ThisGuyCodes Aug 28, 2014

ThisGuyCodes Aug 28, 2014

ThisGuyCodes commented Aug 28, 2014

coveralls commented Aug 28, 2014

coveralls commented Aug 29, 2014

coveralls commented Aug 29, 2014

ThisGuyCodes Sep 2, 2014

coveralls commented Sep 2, 2014

MRigal commented Dec 10, 2014

ThisGuyCodes commented Dec 10, 2014

MRigal commented Dec 10, 2014

ThisGuyCodes commented Dec 10, 2014

MRigal commented Dec 11, 2014

ThisGuyCodes commented Dec 11, 2014

ThisGuyCodes commented Dec 11, 2014

MRigal commented Apr 29, 2015

wojcikstefan commented Dec 4, 2016

let username and password be specified outside a uri #731

let username and password be specified outside a uri #731

Conversation

ThisGuyCodes commented Aug 11, 2014

yograterol commented Aug 11, 2014

yograterol commented Aug 20, 2014

ThisGuyCodes commented Aug 20, 2014

coveralls commented Aug 28, 2014

ThisGuyCodes Aug 28, 2014

Choose a reason for hiding this comment

ThisGuyCodes Aug 28, 2014

Choose a reason for hiding this comment

ThisGuyCodes commented Aug 28, 2014

coveralls commented Aug 28, 2014

coveralls commented Aug 29, 2014

coveralls commented Aug 29, 2014

ThisGuyCodes Sep 2, 2014

Choose a reason for hiding this comment

coveralls commented Sep 2, 2014

MRigal commented Dec 10, 2014

ThisGuyCodes commented Dec 10, 2014

MRigal commented Dec 10, 2014

ThisGuyCodes commented Dec 10, 2014

MRigal commented Dec 11, 2014

ThisGuyCodes commented Dec 11, 2014

ThisGuyCodes commented Dec 11, 2014

MRigal commented Apr 29, 2015

wojcikstefan commented Dec 4, 2016