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

NODE-3152: ensure AWS environment variables are applied properly #2756

Conversation

rose-m
Copy link
Contributor

@rose-m rose-m commented Mar 15, 2021

Description

The AWS environment variables were not detected properly.

The checks for username == null and password == null would always return false as the static merge(...) method that is used to create an instance of MongoCredentials defaulted to an empty string as value for both.

Furthermore, the AWS_SESSION_TOKEN must be applied if it is present in the environment variables and takes precedence over any other value. See https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#environment-variables:

However, if AWS_SESSION_TOKEN is set Drivers MUST use its value as the session token.

The direct assignment of AWS_SESSION_TOKEN would also potentially fail as it happened that this.mechanismProperties was initialized with an already freezed object throwing an error. That's why I changed that to a destructured assignment.

With those changes I am able to run all my E2E tests in mongosh for AWS authentication successfully.

Please note that I did not know where to add test cases for the driver and would ask you to either give me a hint so I can add them or add them on your own :)

@nbbeeken nbbeeken requested review from nbbeeken, durran and emadum March 15, 2021 14:14
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM Thanks @rose-m! I'll merge this soon (edit: Actually I've enabled the AWS Auth tests, so I'll wait on those, and we still have some spec convo above)

Just a heads up to the team: Looks like we need to back port this to 3.6 so I'll put that metadata into the ticket.

@rose-m rose-m force-pushed the NODE-3152-fix-aws-auth-environment-variable-handling branch from c32b473 to 8065618 Compare April 6, 2021 14:57
@nbbeeken nbbeeken merged commit 341a602 into mongodb:4.0 Apr 6, 2021
@rose-m rose-m deleted the NODE-3152-fix-aws-auth-environment-variable-handling branch April 6, 2021 16:16
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.

5 participants