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(NODE-2905): support SERVICE_NAME authentication mechanism property #2857

Merged

Conversation

rose-m
Copy link
Contributor

@rose-m rose-m commented Jun 23, 2021

This PR adds support for the SERVICE_NAME authentication mechanism property that replaces the old gssapiServiceName query string parameter.

It also includes a test case that would set the SERVICE_NAME to an invalid value and verifies the connection consequently fails.

NOTE: I've also started a small README file for the manual tests as I was initially unsure on how to run them locally.

Let me know if there's anything I need to adapt 😊

'Deprecated (or unknown) options are ignored if replacement exists',

// We already handle this case in different ways
'may support deprecated gssapiServiceName option (GSSAPI)'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this as it was the last reference to gssapiServiceName and there is no test with that description anymore.

@rose-m rose-m force-pushed the NODE-2905-support-service-name-auth-property branch from 9229b44 to 539f5eb Compare June 23, 2021 14:28
@dariakp dariakp requested a review from durran June 23, 2021 22:01
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 23, 2021
);
client.connect(function (err) {
expect(err).to.exist;
expect(err.message).to.match(/Error from KDC: LOOKING_UP_SERVER/);
Copy link
Member

Choose a reason for hiding this comment

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

This test is failing on Evergreen with a different error message than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... dumb me did only adapt it for the other test 🙈 should be fixed now!

@rose-m rose-m force-pushed the NODE-2905-support-service-name-auth-property branch from 539f5eb to a3aa420 Compare June 24, 2021 13:15
@durran durran self-requested a review June 24, 2021 14:21
@durran durran requested review from a team, W-A-James, dariakp and nbbeeken and removed request for a team June 24, 2021 14:23
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Thanks for that testing documentation! one small style request, otherwise lgtm so I'll leave a ✅

src/cmap/auth/gssapi.ts Outdated Show resolved Hide resolved
@rose-m rose-m force-pushed the NODE-2905-support-service-name-auth-property branch from a3aa420 to 4235ec2 Compare June 24, 2021 16:23
test/manual/kerberos.test.js Outdated Show resolved Hide resolved
@rose-m rose-m force-pushed the NODE-2905-support-service-name-auth-property branch from 4235ec2 to 26c5367 Compare June 25, 2021 13:24
@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 25, 2021
@durran durran merged commit dfb91b8 into mongodb:4.0 Jun 25, 2021
@rose-m rose-m deleted the NODE-2905-support-service-name-auth-property branch June 26, 2021 10:14
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
4 participants