-
Notifications
You must be signed in to change notification settings - Fork 1.8k
test(NODE-4338): refactor auth prose tests #3859
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
Conversation
3fcd76a
to
ccd5854
Compare
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Infrastructure as Code | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Vulnerabilities | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
5286b94
to
8cac583
Compare
if ( | ||
process.env.AUTH === 'noauth' || | ||
process.env.LOAD_BALANCER || | ||
!satisfies(this.configuration.version, '>=3.7.3') |
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.
Note the create user commands will fail on < 3.7.3 since mechanisms
cannot be provided to the command.
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.
every test already specifies a min server version. do we want to just move this logic into tests requires
? We can also use the generic predicate filter
const metadata: MongoDBMetadataUI = {
requires: {
auth: 'enabled',
mongodb: '>=3.7.3',
predicate: () => process.env.LOAD_BALANCER ? 'TODO(NODE-5631): fix tests to run in load balancer mode.' : true
}
}
....
it('description', metadata, async 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.
Updated to suggestion.
[ | ||
{ username: 'IX', password: 'IX' }, | ||
{ username: 'IX', password: 'I\u00ADX' }, | ||
{ username: '\u2168', password: 'IV' }, | ||
{ username: '\u2168', password: 'I\u00ADV' } | ||
].forEach(({ username, password }) => { |
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.
[ | |
{ username: 'IX', password: 'IX' }, | |
{ username: 'IX', password: 'I\u00ADX' }, | |
{ username: '\u2168', password: 'IV' }, | |
{ username: '\u2168', password: 'I\u00ADV' } | |
].forEach(({ username, password }) => { | |
for (const { username, password } of [ | |
{ username: 'IX', password: 'IX' }, | |
{ username: 'IX', password: 'I\u00ADX' }, | |
{ username: '\u2168', password: 'IV' }, | |
{ username: '\u2168', password: 'I\u00ADV' } | |
]) { |
optional code modernization
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.
Done
if ( | ||
process.env.AUTH === 'noauth' || | ||
process.env.LOAD_BALANCER || | ||
!satisfies(this.configuration.version, '>=3.7.3') |
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.
every test already specifies a min server version. do we want to just move this logic into tests requires
? We can also use the generic predicate filter
const metadata: MongoDBMetadataUI = {
requires: {
auth: 'enabled',
mongodb: '>=3.7.3',
predicate: () => process.env.LOAD_BALANCER ? 'TODO(NODE-5631): fix tests to run in load balancer mode.' : true
}
}
....
it('description', metadata, async function() { ...} );
auth: { | ||
username: userMap.sha256.username, | ||
password: userMap.sha256.password | ||
}, | ||
authSource: 'admin', | ||
authMechanism: 'SCRAM-SHA-1' |
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.
Should this be updated to use the current user, instead of always hard-coding to sha256
?
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.
It's basically explicitly stating that the sha256
only user cannot authenticate when specifying SCRAM-SHA-1
as the mechanism. I think it's clearer this way.
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.
* Step 3 * For test users that support only one mechanism, verify that explictly specifying * the other mechanism fails.
The test implies that we also need to test that the SCRAM-SHA-1
user cannot authenticate by specifying SCRAM-SHA-256
. Right now, this only covers the opposite.
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.
I've added the reverse test now.
it( | ||
'sends speculativeAuthenticate on initial handshake on MongoDB 4.4+', | ||
metadata, | ||
async 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.
I had to read the spec here to clarify:
Older servers will ignore the speculativeAuthenticate argument. New servers will participate in the standard authentication conversation if this argument is missing.
I don't think we have any branching logic determining when we use speculative authentication. Maybe we should remove the server version from the test?
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.
I've removed the 4.4+ version from the description but left the metadata there as all tests still need 3.7.3+.
auth: { | ||
username: userMap.sha256.username, | ||
password: userMap.sha256.password | ||
}, | ||
authSource: 'admin', | ||
authMechanism: 'SCRAM-SHA-1' |
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.
* Step 3 * For test users that support only one mechanism, verify that explictly specifying * the other mechanism fails.
The test implies that we also need to test that the SCRAM-SHA-1
user cannot authenticate by specifying SCRAM-SHA-256
. Right now, this only covers the opposite.
Description
Re-enables SCRAM auth tests and refactors to not use
withClient
.What is changing?
withClient
Is there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-4338
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript