fix: Avoids sending database user password in update request if the value has not changed #2005

AgustinBettati · 2024-03-11T12:47:28Z

Description

Link to any related issue(s): CLOUDP-235738

This change aligns with our behaviour prior to version 1.12.0.
Additionally a warning is included to bring visibility on the suggested course of action after a database user is created:

mongodbatlas_database_user.static_user: Creation complete after 7s [id=YXV0aF9kYXRhYmFzZV9uYW1l:YWRtaW4=-cHJvamVjdF9pZA==:NjUyNTE0NDZhZTVmM2Y2ZWM3OTY4YjEz-dXNlcm5hbWU=:dXNlci10aGF0LWNhbi1iZS1kZWxldGVkLTM=]
╷
│ Warning: If the password value will be managed externally it is advised to remove the attribute
│ 
│   with mongodbatlas_database_user.static_user,
│   on main.tf line 72, in resource "mongodbatlas_database_user" "static_user":
│   72: resource "mongodbatlas_database_user" "static_user" {
│ 
│ More details can be found in resource documentation under the 'password' attribute

Manual verification that PATCH request is being generated correctly

Generated patch request when updating a labels value but password remains the same, password is not sent:

PATCH /api/atlas/v2/groups/65251446ae5f3f6ec7968b13/databaseUsers/admin/user-that-can-be-deleted-2 HTTP/1.1
{
 "awsIAMType": "NONE",
 "databaseName": "admin",
 "groupId": "65251446ae5f3f6ec7968b13",
 "labels": [
  {
   "key": "My Key2",
   "value": "My Value"
  }
 ],
 "ldapAuthType": "NONE",
 "oidcAuthType": "NONE",
 "roles": [
  {
   "databaseName": "admin",
   "roleName": "readAnyDatabase"
  }
 ],
 "scopes": [],
 "username": "user-that-can-be-deleted-2",
 "x509Type": "NONE"
}

Generated patch request when updating password value, password is sent:

PATCH /api/atlas/v2/groups/65251446ae5f3f6ec7968b13/databaseUsers/admin/user-that-can-be-deleted-2 HTTP/1.1
{
 "awsIAMType": "NONE",
 "databaseName": "admin",
 "groupId": "65251446ae5f3f6ec7968b13",
 "labels": [
  {
   "key": "My Key3",
   "value": "My Value"
  }
 ],
 "ldapAuthType": "NONE",
 "oidcAuthType": "NONE",
 "password": "udpatedpassword",
 "roles": [
  {
   "databaseName": "admin",
   "roleName": "readAnyDatabase"
  }
 ],
 "scopes": [],
 "username": "user-that-can-be-deleted-2",
 "x509Type": "NONE"
}

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

…alue has not changed

lantoli · 2024-03-11T13:30:09Z

internal/service/databaseuser/model_database_user.go

-	}, nil
+	}
+
+	if statePasswordValue != dbUserModel.Password {


i guess this also covers if the password attribute is deleted, then we set nil so it's not sent

correct, added an additional unit test to cover this case as well, thanks

lantoli · 2024-03-11T13:31:04Z

internal/service/databaseuser/resource_database_user.go

@@ -231,6 +231,8 @@ func (r *databaseUserRS) Create(ctx context.Context, req resource.CreateRequest,
 		return
 	}

+	resp.Diagnostics.AddWarning("If the password value will be managed externally it is advised to remove the attribute", "More details can be found in resource documentation under the 'password' attribute")


is this warning shown always or it has some condition?

will always be shown, not really any particular condition we can use because password will always be defined for create operation (fails in API otherwise)

…not be sent in patch request

andreaangiolillo

LGTM

andreaangiolillo · 2024-03-11T13:55:12Z

website/docs/r/database_user.html.markdown

 * `username` - (Required) Username for authenticating to MongoDB. USER_ARN or ROLE_ARN if `aws_iam_type` is USER or ROLE.
-* `password` - (Required) User's initial password. A value is required to create the database user, however the argument but may be removed from your Terraform configuration after user creation without impacting the user, password or Terraform management. IMPORTANT --- Passwords may show up in Terraform related logs and it will be stored in the Terraform state file as plain-text. Password can be changed after creation using your preferred method, e.g. via the MongoDB Atlas UI, to ensure security.  If you do change management of the password to outside of Terraform be sure to remove the argument from the Terraform configuration so it is not inadvertently updated to the original password.
+* `password` - (Required) User's initial password. A value is required to create the database user, however the argument may be removed from your Terraform configuration after user creation without impacting the user, password or Terraform management. If you do change management of the password to outside of Terraform it is advised to remove the argument from the Terraform configuration. IMPORTANT --- Passwords may show up in Terraform related logs and it will be stored in the Terraform state file as plain-text. Password can be changed after creation using your preferred method, e.g. via the MongoDB Atlas UI, to ensure security.


Should we add a warning at the top of this page to make sure that this warning message is not missed?

makes sense, included.

include warning during creation

2741f70

github-actions bot added the bug label Mar 11, 2024

AgustinBettati force-pushed the CLOUDP-235738 branch 2 times, most recently from e55360d to 436dca6 Compare March 11, 2024 13:01

fix: Avoids sending database user password in update request if the v…

6a0b414

…alue has not changed

AgustinBettati force-pushed the CLOUDP-235738 branch from 436dca6 to 6a0b414 Compare March 11, 2024 13:07

AgustinBettati marked this pull request as ready for review March 11, 2024 13:24

AgustinBettati requested a review from a team as a code owner March 11, 2024 13:24

lantoli reviewed Mar 11, 2024

View reviewed changes

lantoli approved these changes Mar 11, 2024

View reviewed changes

add unit test for case when password attribute is removed and should …

b01d5d1

…not be sent in patch request

maastha approved these changes Mar 11, 2024

View reviewed changes

andreaangiolillo approved these changes Mar 11, 2024

View reviewed changes

include warning message

a93ffea

AgustinBettati merged commit 29d4268 into master Mar 12, 2024
45 checks passed

AgustinBettati deleted the CLOUDP-235738 branch March 12, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Avoids sending database user password in update request if the value has not changed #2005

fix: Avoids sending database user password in update request if the value has not changed #2005

AgustinBettati commented Mar 11, 2024 •

edited

Loading

lantoli Mar 11, 2024

AgustinBettati Mar 11, 2024

lantoli Mar 11, 2024

AgustinBettati Mar 11, 2024

andreaangiolillo left a comment

andreaangiolillo Mar 11, 2024

AgustinBettati Mar 11, 2024

fix: Avoids sending database user password in update request if the value has not changed #2005

fix: Avoids sending database user password in update request if the value has not changed #2005

Conversation

AgustinBettati commented Mar 11, 2024 • edited Loading

Description

Manual verification that PATCH request is being generated correctly

Type of change:

Required Checklist:

Further comments

lantoli Mar 11, 2024

Choose a reason for hiding this comment

AgustinBettati Mar 11, 2024

Choose a reason for hiding this comment

lantoli Mar 11, 2024

Choose a reason for hiding this comment

AgustinBettati Mar 11, 2024

Choose a reason for hiding this comment

andreaangiolillo left a comment

Choose a reason for hiding this comment

andreaangiolillo Mar 11, 2024

Choose a reason for hiding this comment

AgustinBettati Mar 11, 2024

Choose a reason for hiding this comment

AgustinBettati commented Mar 11, 2024 •

edited

Loading