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

INTPYTHON-424 Prevent redundant requirement for USER and PASSWORD arguments when URI with authentication is provided #194

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Jibola
Copy link
Collaborator

@Jibola Jibola commented Nov 21, 2024

No description provided.

@timgraham
Copy link
Collaborator

Django doesn't natively support passing URLs in HOST. There is dj-database-url to parse a URL into the DATABASES dictionary format that Django expects.

If we go down the road of allowing HOST to contain USER and PASSWORD, we'd need to audit this package (e.g.

host = settings_dict["HOST"]
port = settings_dict["PORT"]
dbname = settings_dict["NAME"]
user = settings_dict["USER"]
passwd = settings_dict["PASSWORD"]
) and Django itself for usages of these values to make sure both ways user/password could be provided are accounted for. I don't like the extra complexity this introduces. It's possible this could cause incompatibilities with third-party packages that assume the usual DATABASES values as well.

@aclark4life
Copy link
Collaborator

Right, although as @Jibola mentioned to me there is some precedent for this, in that HOST can take things-that-aren't-a-hostname. Things-that-are-credentials may be too much to ask though.

Also note that this came up because I tried to use mongodb+srv (with Atlas) and PyMongo only supports SRV via the URI syntax. I don't like that either, but we're not going to fix that any time soon or possibly ever.

If we were to merge this, the URI could be passed in without having to "redundantly specify user and pass" and PyMongo will correctly parse the URI and SRV connections will work "as expected".

If we don't merge this, we still have to support SRV connections so I propose the following changes to our backend:

  • Raise InvalidHostError if HOST is not a hostname
  • Add is_srv parameter to OPTIONS which if True we add mongodb+srv:// to the connection string passed to PyMongo.
  • Add support to dj-database-url to include { "OPTIONS" { "is_srv": True }} if mongodb+srv:// is found in the URI.

django_mongodb/base.py Outdated Show resolved Hide resolved
@timgraham
Copy link
Collaborator

If we want the user to be able to configure this backend with something like, mongodb+srv://mycluster.abcd1.mongodb.net/myFirstDatabase (which seems to be a requirement), then I think we need to parse it using either dj-database-url or, if we don't want the external dependency, a utility in this library.

From a Django perspective, I think it'll be problematic for the user to provide a "complex" URI that contains user, password, and/or database as the DATABASES['HOST'] because of code that assumes values like database name are parsed out.

@Jibola
Copy link
Collaborator Author

Jibola commented Nov 22, 2024

If we want the user to be able to configure this backend with something like, mongodb+srv://mycluster.abcd1.mongodb.net/myFirstDatabase (which seems to be a requirement), then I think we need to parse it using either dj-database-url or, if we don't want the external dependency, a utility in this library.

From a Django perspective, I think it'll be problematic for the user to provide a "complex" URI that contains user, password, and/or database as the DATABASES['HOST'] because of code that assumes values like database name are parsed out.

We can make a small wrapper around the mongodb parse_uri functionality and let folks use that if they'd like. https://pymongo.readthedocs.io/en/stable/api/pymongo/uri_parser.html#pymongo.uri_parser.parse_uri

@timgraham
Copy link
Collaborator

Right, where the wrapper takes the parsed result and translates into the format of Django's settings.DATABASES.

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