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

test(NODE-4338): refactor auth prose tests #3859

Merged
merged 6 commits into from
Sep 20, 2023
Merged

test(NODE-4338): refactor auth prose tests #3859

merged 6 commits into from
Sep 20, 2023

Conversation

durran
Copy link
Member

@durran durran commented Sep 6, 2023

Description

Re-enables SCRAM auth tests and refactors to not use withClient.

What is changing?

  • Refactors the SCRAM auth tests to not use withClient
  • Skips failing unicode username/password tests.
Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-4338

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@durran durran force-pushed the NODE-4338 branch 9 times, most recently from 3fcd76a to ccd5854 Compare September 12, 2023 13:16
Copy link

@orca-security-us orca-security-us bot left a 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
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@durran durran force-pushed the NODE-4338 branch 16 times, most recently from 5286b94 to 8cac583 Compare September 14, 2023 13:49
@durran durran marked this pull request as ready for review September 14, 2023 14:23
if (
process.env.AUTH === 'noauth' ||
process.env.LOAD_BALANCER ||
!satisfies(this.configuration.version, '>=3.7.3')
Copy link
Member Author

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.

Copy link
Contributor

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() { ...} );

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to suggestion.

@baileympearson baileympearson self-assigned this Sep 14, 2023
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 14, 2023
Comment on lines 342 to 347
[
{ username: 'IX', password: 'IX' },
{ username: 'IX', password: 'I\u00ADX' },
{ username: '\u2168', password: 'IV' },
{ username: '\u2168', password: 'I\u00ADV' }
].forEach(({ username, password }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[
{ 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

Copy link
Member Author

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')
Copy link
Contributor

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() { ...} );

test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
Comment on lines +195 to +197
auth: {
username: userMap.sha256.username,
password: userMap.sha256.password
},
authSource: 'admin',
authMechanism: 'SCRAM-SHA-1'
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines 278 to 282
it(
'sends speculativeAuthenticate on initial handshake on MongoDB 4.4+',
metadata,
async function () {
Copy link
Contributor

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?

Copy link
Member Author

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+.

Comment on lines +195 to +197
auth: {
username: userMap.sha256.username,
password: userMap.sha256.password
},
authSource: 'admin',
authMechanism: 'SCRAM-SHA-1'
Copy link
Contributor

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.

baileympearson
baileympearson previously approved these changes Sep 18, 2023
@baileympearson baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 18, 2023
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
test/integration/auth/auth.prose.test.ts Outdated Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Sep 19, 2023
@baileympearson baileympearson merged commit 3932260 into main Sep 20, 2023
24 of 26 checks passed
@baileympearson baileympearson deleted the NODE-4338 branch September 20, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants