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

fix: Remove incorrect authSource param from MONGODB_URL environment variable #458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

malhusseiny
Copy link

@malhusseiny malhusseiny commented Mar 27, 2024

This fix aims to address the issue with the incorrect MongoDB connection string specified in the MONGODB_URL environment variable, as generated by the CLI. #451

Consider a Docker Compose configuration for a MongoDB service as follows:

mongodb:
  image: mongo:latest
  environment:
    MONGO_INITDB_ROOT_USERNAME: app
    MONGO_INITDB_ROOT_PASSWORD: changeMe
    MONGO_INITDB_DATABASE: symfony

This setup results in the symfony var:export command generating a MONGODB_URL environment variable similar to:

export MONGODB_URL=mongodb://admin:changeMe@127.0.0.1:27017/?authSource=symfony

Such a configuration typically leads to authentication failures for most applications attempting to connect to MongoDB.

To address this, I have chosen to omit the "path" portion of the connection string entirely, rather than merely replacing /authSource=symfony with /symfony. The latter approach would not resolve the underlying issue. In MongoDB connection strings, specifying a default working database in the URL path (e.g., mongodb://user:pass@127.0.0.1:27017/defaultDb) uses defaultDb as both the default and authentication database. This behavior is often unintended. To correctly designate separate databases for default operations and authentication, the authSource parameter must be explicitly added (e.g., mongodb://user:pass@127.0.0.1:27017/defaultDb?authSource=authDb). However, this necessitates the declaration of an authSource database within the Docker Compose configuration. I propose avoiding this complication entirely, as both the default database and the authSource can be separately specified through dedicated options (e.g., in Doctrine) without embedding them in the connection string.

@fabpot
Copy link
Contributor

fabpot commented Mar 27, 2024

@alcaeus Can you confirm that this is the right thing to do?

@tucksaun
Copy link
Contributor

I see why not defining the authSource might make sense.
Yet, the official Docker image seems to hardcode the authentication database to be admin (according to their Docker Hub doc and the entrypoint) so maybe we could define it when we detected some credentials to make it a smooth experience for most users?

Regarding the main database I'm not sure to understand why we should not define it. Because if I get that correctly this means this will make any user of mongodb required to explicitly specify it in Doctrine's configuration, right?

@tucksaun
Copy link
Contributor

I think it might also be interesting to ping @quentint who initially contributed the addition of the MONGODB_URL definition :)

@jmikola
Copy link

jmikola commented Mar 28, 2024

👋 @alcaeus is on holiday so @GromNaN summoned me instead.

Consider a Docker Compose configuration for a MongoDB service as follows...

What is the purpose of MONGO_INITDB_DATABASE in the Docker Compose configuration if it's not utilized for the authentication database? Is that meant to be a default database for the application to use? If so, does that mean Docker always creates users in the admin database (perhaps so according to docker-entrypoint.sh, which @tucksaun shared).

In MongoDB connection strings, specifying a default working database in the URL path (e.g., mongodb://user:pass@127.0.0.1:27017/defaultDb) uses defaultDb as both the default and authentication database.

With respect to the PHP driver, there is no notion of a "default database" and it'd be helpful to only consider an "auth database" (with preference given to the authSource URI option over the "path" component in the URI. The MongoDB Connection String spec is the authority here.

That said, some other drivers/libraries may still infer a default database from the "path" component in the URI (see mongodb/mongo-php-driver#1470 (comment) for more context on that); however, this isn't relevant to Doctrine ODM.

According to DoctrineMongoDBExtension::loadConnections(), the server property for a connection is only used to construct the PHP library's MongoDB\Client object. The connection string seems entirely unrelated to any default database utilized for Doctrine ODM models.

In the absence of specifying a database name in the model metadata, ODM relies on Configuration::getDefaultDB(), and falls back to 'doctrine' (see: DocumentManager::getDocumentDatabase()). When using the Symfony bundle, Configuration::setDefaultDB() should always be called with something. If the application doesn't explicitly define default_database in the bundle config, the DIC configuration uses 'default' (see: Configuration::getConfigTreeBuilder()).

Yet, the official Docker image seems to hardcode the authentication database to be admin (according to their Docker Hub doc and the entrypoint) so maybe we could define it when we detected some credentials to make it a smooth experience for most users?

I see no reason to specify 'admin' as a default value, as drivers already use that as a default for database-related auth mechanisms (see: MongoDB Auth spec. Note that other auth mechanisms might use a different value (e.g. $external), but I expect those wouldn't come up with the Docker container being used here.


Some other thoughts:

I don't understand what the isMaster() function (called just above the diff in this PR) does. The function dates back to the first commit for envs.go in 5ac4bd9 and originally applied to pgsql, mysql, and mongodb schemes. It was later removed for pgsql and mysql in 1c0b3c8 but remains in place for MongoDB.

I also don't understand what the other environment variables are used for. MONGO_NAME, MONGO_DATABASE, and MONGO_DB are all still set to endpoint["path"]. Since we previously established that the "path" URI component is not a default database but merely a deprecated way of specifying the auth databsae, I wouldn't expect it to be used for any other purpose. And even if it was desirable to expose the "path" as an environment variable, assigning it to three different vars seems redundant and more likely to mislead users. This question relates more to the paste that @malhusseiny included in #451.

Going back to my original question about MONGO_INITDB_DATABASE in the Docker config. Perhaps that would be a sensible value to use for one of these other environment variables (if it is accessible within envs.go). If MONGO_INITDB_DATABASE is intended to be the application database, Symfony users would likely want to use the same value for the default_database option in the DoctrineMongoDBBundle config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants