Skip to content

Conversation

@aljones15
Copy link
Contributor

@aljones15 aljones15 commented Sep 2, 2022

This was tested locally with a server with authentication on.
The following was the result:

 MONGODB_URL="mongodb://bedrock_mongodb_test-user:password@localhost:27017/bedrock_mongodb_test" npm t
{
  url: 'mongodb://bedrock_mongodb_test-user:password@localhost:27017/bedrock_mongodb_test',
  connectOptions: {
    serverSelectionTimeoutMS: 30000,
    promoteBuffers: true,
    forceServerObjectId: true,
    writeConcern: { w: 'majority', j: true }
  }
}
... test results all passed

Additionally I tested what happens if someone sets connectOptions.auth

 MONGODB_USERNAME=bedrock_mongodb_test-user MONGODB_PASSWORD=password npm t

> bedrock-mongodb-test@0.0.1-0 test
> node --preserve-symlinks test.js test

{
  url: 'mongodb://localhost:27017/bedrock_mongodb_test',
  connectOptions: {
    serverSelectionTimeoutMS: 30000,
    promoteBuffers: true,
    forceServerObjectId: true,
    auth: { username: 'bedrock_mongodb_test-user', password: 'password' },
    writeConcern: { w: 'majority', j: true }
  }
}
... all tests passing
  1. Removes config.mongodb.username
  2. Removes config.mongodb.password
  3. Removes authn checks from connect
  4. Renames ./lib/authn.js to ./lib/connect.js
  5. Removes auth details from urls.create
  6. Updates auth tests to use a connection string
  7. Drops support for node v14 from ci actions (reduces ci action minutes used)

PR Mystery that needs to be solved:

  • I was able to connect to a local auth enabled mongodb server with the options above using the auth object, but the ci tests failed which suggests there is an edge case where the auth object is failing when the authSource is not the database being connected to.

Resolves this issue, by removing auth defaults: #54

@aljones15 aljones15 self-assigned this Sep 2, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #93 (ad561c7) into mongo-driver-4-rc (1b4c0e6) will increase coverage by 2.40%.
The diff coverage is 50.00%.

@@                  Coverage Diff                  @@
##           mongo-driver-4-rc      #93      +/-   ##
=====================================================
+ Coverage              87.08%   89.48%   +2.40%     
=====================================================
  Files                      8        8              
  Lines                    813      742      -71     
=====================================================
- Hits                     708      664      -44     
+ Misses                   105       78      -27     
Impacted Files Coverage Δ
lib/authn.js 83.72% <ø> (+6.67%) ⬆️
lib/index.js 81.85% <0.00%> (+0.71%) ⬆️
lib/test.config.js 100.00% <ø> (ø)
lib/urls.js 91.48% <ø> (+6.30%) ⬆️
lib/config.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aljones15 aljones15 force-pushed the mongo-driver-4-rc-no-auth-defaults branch from c9af7a5 to 43e0cf9 Compare November 28, 2022 21:02
@aljones15 aljones15 marked this pull request as ready for review January 6, 2023 00:37
- **BREAKING**: Remove all defaults for `authSource`.
- **BREAKING**: Remove `config.mongodb.username`.
- **BREAKING**: Remove `config.mongodb.password`.
- **BREAKING**: Remove setting username, password, and or authSource from `urls.create`.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add something here that says that all of these options, if needed, must be provided via the database url config option?

bedrock.config.mongodb.password = 'password'; // default: password

// the mongodb database 'my_project_dev' and the 'my_project' user will
// be created on start up following a prompt for the admin user credentials
Copy link
Member

Choose a reason for hiding this comment

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

These comments need to be brought into sync with these changes.

connectOptions.auth = {
username: 'me',
password: 'password'
};
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to support this, only url options, right?

// the `authSource` is database to authenticate against
// it should be specified or it will default to the database
// you're connecting to
connectOptions.authSource = 'my_provider_auth_db';
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed too, right? It looks like it can be done via the url. We want there to be just one way to do these things if possible.

// omit a username and password if those are provided as
// `config.mongodb.username` and `config.mongodb.password`, otherwise those
// `config.mongodb.connnectOptions.auth.username` and
// `config.mongodb.connectOptions.auth.password`, otherwise those
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to bring these comments in sync after removing the extra options.

@dlongley
Copy link
Member

dlongley commented Mar 5, 2025

Closing in favor of #107.

@dlongley dlongley closed this Mar 5, 2025
@dlongley dlongley deleted the mongo-driver-4-rc-no-auth-defaults branch March 5, 2025 22:41
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.

4 participants