-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-7047)!: use custom credential provider first after URI #4656
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
base: main
Are you sure you want to change the base?
Conversation
1a59a5a
to
6e98b5d
Compare
7281459
to
8271c1f
Compare
let provider; | ||
|
||
beforeEach(function () { | ||
console.log(client?.options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray debugging console log here and in L222?
expect(providerCount).to.be.greaterThan(0); | ||
beforeEach(function () { | ||
if (client?.options.credentials.username || !process.env.AWS_ACCESS_KEY_ID) { | ||
this.skipReason = 'Test only runs when credentials are present in the environment'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"... and not the URI"? Is that what client?.options.credentials.username
is testing for? If so, should there be a similar check in the case above to make sure that process.env.AWS_ACCESS_KEY_ID
is not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"... and not the URI"? Is that what client?.options.credentials.username is testing for?
Yup
If so, should there be a similar check in the case above to make sure that process.env.AWS_ACCESS_KEY_ID is not set?
No - URI takes precedence over everything, so it should run both when there are no credentials in the environment and when there are credentials in the environment.
|
||
expect(client).to.have.nested.property('s.authProviders'); | ||
const provider = client.s.authProviders.getOrCreateProvider('MONGODB-AWS'); | ||
const provider = client.s.authProviders.getOrCreateProvider('MONGODB-AWS', {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what prompted this change?
this.mechanism = options.mechanism || AuthMechanism.MONGODB_DEFAULT; | ||
this.mechanismProperties = options.mechanismProperties || {}; | ||
|
||
if (this.mechanism.match(/MONGODB-AWS/i)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work because we outsource the env credential reading to the sdk as of NODE-6988 and previously we were overriding the natural order by explicitly reading in the env vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, reading credentials from the environment in addition to using the SDK has always been a bug (https://jira.mongodb.org/browse/NODE-6987)
AWS_CREDENTIAL_PROVIDER: provider | ||
} | ||
context('2. Custom Credential Provider Authentication Precedence', function () { | ||
context('Case 1: Credentials in URI Take Precedence', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all the prose tests, we usually copy the language of the prose test in the comments, so that it's easier to tell how the test implementation maps to the prose and and also to identify cases of divergence between the spec prose and the implementation (if either the prose or the implementation are updated in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment in addition to Daria's comment
}); | ||
|
||
it('authenticates with a user provided credentials provider', async function () { | ||
console.log(process.env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(process.env); |
Description
Updates AWS auth to use a custom credential provider first after URI/MongoClient credentials if present.
Summary of Changes
What is the motivation for this change?
NODE-7047
Release Highlight
Custom AWS Credential Provider Takes Highest Precedence
When providing a custom AWS credential provider via the auth mechanism property
AWS_CREDENTIAL_PROVIDER
, it will now take the highest precedence over any other AWS auth mechanism.Double check the following
npm run check:lint
)type(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript