Skip to content

Conversation

@kevinAlbs
Copy link
Collaborator

Summary

Remove code for the MONGODB-CR auth mechanism

Verified with this patch build.

Background & Motivation

MONGODB-CR no longer supported

MongoDB server 4.0 dropped support of the authentication mechanism MONGODB-CR. Quoting DRIVERS-441:

MONGODB-CR was deprecated in MongoDB 3.6.0 and was removed in MongoDB 3.7.1

As of CDRIVER-4815, the C driver supports the minimum server version 4.0. Attempting to connect to older servers results in error:

% ./cmake-build/src/libmongoc/example-client 
Cursor Failure: Server at 127.0.0.1:27017 reports wire version 6, but this version of libmongoc requires at least 7 (MongoDB 4.0)

Authenticating with MONGODB-CR is not possible on supported servers. Using MONGODB-CR on a newer server results in a protocol error:

% ./cmake-build/src/libmongoc/example-client "mongodb://foo:bar@localhost:27017/?authMechanism=MONGODB-CR"
Cursor Failure: no such command: 'getnonce'

With changes in this PR, a client-side error is now returned:

% ./cmake-build/src/libmongoc/example-client "mongodb://foo:bar@localhost:27017/?authMechanism=MONGODB-CR"
Cursor Failure: Unknown authentication mechanism "MONGODB-CR".

Skipping tests

Auth spec tests are updated to mongodb/specifications@82be6f2 to remove references to MONGODB-CR. Tests requiring unimplemented features or fixes to known bugs are skipped. test_skip_t is extended to support skipping from a description substring to ease skipping all MONGODB-OIDC tests.

To be used to skip all "MONGODB-OIDC" tests. OIDC is not-yet implemented.
The driver test server uses the auth source named "mongodb-cr", but uses SCRAM-SHA-1.
@kevinAlbs kevinAlbs marked this pull request as ready for review November 14, 2024 20:32
@kevinAlbs kevinAlbs requested review from a user and eramongodb November 14, 2024 20:32

typedef struct {
const char *description;
bool check_substring;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this placed between existing data members instead of after?

Copy link
Collaborator Author

@kevinAlbs kevinAlbs Nov 14, 2024

Choose a reason for hiding this comment

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

To locate it next to description, since check_substring applies to description. But I can move to the end if desired. Added comment to describe the purpose of check_substring.

Copy link
Contributor

@eramongodb eramongodb Nov 14, 2024

Choose a reason for hiding this comment

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

I would normally recommend moving it to the end of the struct so existing code is not impacted as much (i.e. in test-mongoc-retryable-writes.c), but I think using designated initializers is preferable anyways in these circumstances (+ they can be ordered however is preferable, as already done in test-mongoc-connection-uri.c), so I do not mind the proposed changes as-is, other than perhaps to better conform to -Wpadded-friendly layout practices:

warning: padding struct 'test_skip_t' with 7 bytes to align 'reason' [-Wpadded]
   36 |    const char *reason;
      |                ^

vs. moving it to the end of the struct (no substantial difference in this case): ¯\_(ツ)_/¯

warning: padding size of 'test_skip_t' with 7 bytes to alignment boundary [-Wpadded]
   33 | typedef struct {
      |         ^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will keep as-is.

kevinAlbs and others added 2 commits November 14, 2024 16:05
@kevinAlbs kevinAlbs requested a review from eramongodb November 14, 2024 21:12

typedef struct {
const char *description;
bool check_substring;
Copy link
Contributor

@eramongodb eramongodb Nov 14, 2024

Choose a reason for hiding this comment

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

I would normally recommend moving it to the end of the struct so existing code is not impacted as much (i.e. in test-mongoc-retryable-writes.c), but I think using designated initializers is preferable anyways in these circumstances (+ they can be ordered however is preferable, as already done in test-mongoc-connection-uri.c), so I do not mind the proposed changes as-is, other than perhaps to better conform to -Wpadded-friendly layout practices:

warning: padding struct 'test_skip_t' with 7 bytes to align 'reason' [-Wpadded]
   36 |    const char *reason;
      |                ^

vs. moving it to the end of the struct (no substantial difference in this case): ¯\_(ツ)_/¯

warning: padding size of 'test_skip_t' with 7 bytes to alignment boundary [-Wpadded]
   33 | typedef struct {
      |         ^

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good to me. Re struct field order: I'm happy with optimizing for semantics rather than memory layout in test code, it's neither something that needs to be fast nor a thing we'll be stuck with if we feel like changing it later.

@kevinAlbs kevinAlbs merged commit 4ccd9db into mongodb:master Nov 15, 2024
22 of 45 checks passed
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.

2 participants