Repository: micromdm/micromdm
Pull request #743 (2021-04-23 18:52:04)

Fix SetFirmwarePassword and VerifyFirmwarePassword parameters
Fix for Issue #703. I'm not really sure how to add unit tests, but I did perform the following on a test MacBook:

| Current Local Password | CurrentPassword | NewPassword | Result | Notes |
| -------------------------------- | ----------------------- | ------------------- | --------- | -------- |
| test | *empty* | test | Current password not provided | |
| test | test | test | **PasswordChanged** | |
| test | test | *empty* | A firmware password change is already pending. Machine must be rebooted before any additional change requests are accepted. | Oops - good to know |
| test | test | *empty* | **PasswordChanged** | After reboot |
| *empty* | test | test | **PasswordChanged** | If local password is blank, it doesn't matter what you pass in CurrentPassword |

Seems like everything is working.

jessepeterson: Thanks for that — and the table with results! Are you interested in learning to add a test case? It's not too hard. If you take a look at #669 in `mdm/mdm/mdm_command_test.go` where I added tests to test for specific input/output in plists. Are you familiar with `go test`? It's one of easiest unit test systems to get started with!
korylprince: @jessepeterson I've [written tests before](https://github.com/korylprince/go-ad-auth/blob/master/auth_test.go). I looked through `mdm_command_test.go`, but I'm having trouble understanding exactly what is being tested...
korylprince: Okay, I stared at it long enough to see what the tests were doing, and pushed those. Not sure if the added in tests in `TestMarshalCommand` were necessary, though.
jessepeterson: @korylprince awesome! sorry didn't mean to assume anything. the one test case I'd suggest adding (which this fix was for) would be to add an empty password, and make sure the plist output represents an empty-but-present string value :)
korylprince: I didn't mean to seem offended by your question! If I'm being honest, it's a Friday afternoon, I'm tired, and I didn't feel like writing tests... But it's good practice so I kicked myself into gear :)

If I'm understanding it correctly the `SetFirmwarePassword_NoNewPassword` test I added should test for the issue case. `new_password` is not in `requestBytes`, but `NewPassword` is checked for in the output.

Do you want me to remove the rest of the test cases?
jessepeterson: > If I'm understanding it correctly the SetFirmwarePassword_NoNewPassword test I added should test for the issue case. new_password is not in requestBytes, but NewPassword is checked for in the output.

Yes but you *set* NewPassword in the json, meaning it would be expected to be in the output no matter what. If NewPassword were _empty_/not set at all it should _still_ show up in the output plist (just, as an empty string). This wasn't happening Does that make sense?

> Do you want me to remove the rest of the test cases?

Nah, leave 'em. More tests are good.
korylprince: @jessepeterson I thing you're looking at the `SetFirmwarePassword_NewPassword` test. In the `SetFirmwarePassword_NoNewPassword` I only set `current_password` in the json, not `new_password`:

```go
			requestBytes: []byte(
				`{"request_type":"SetFirmwarePassword","current_password":"test"}`,
			),
```

Unless I'm just really misunderstanding how these tests run...
jessepeterson: Yes indeed! I appear to be blind. Cheers. :)

Patch:
diff --git a/mdm/mdm/command.go b/mdm/mdm/command.go
index dbbd9f9..16e2b81 100644
--- a/mdm/mdm/command.go
+++ b/mdm/mdm/command.go
@@ -243,12 +243,12 @@ type ManagedApplicationFeedback struct {
 
 type SetFirmwarePassword struct {
 	CurrentPassword string `plist:",omitempty" json:"current_password,omitempty"`
-	NewPassword     string `plist:",omitempty" json:"new_password,omitempty"`
+	NewPassword     string `json:"new_password"`
 	AllowOroms      bool   `plist:",omitempty" json:"allow_oroms,omitempty"`
 }
 
 type VerifyFirmwarePassword struct {
-	Password string `plist:",omitempty" json:"password,omitempty"`
+	Password string `json:"password"`
 }
 
 type SetAutoAdminPassword struct {


Test patch:
diff --git a/mdm/mdm/mdm_command_test.go b/mdm/mdm/mdm_command_test.go
index 083711b..84a3efb 100644
--- a/mdm/mdm/mdm_command_test.go
+++ b/mdm/mdm/mdm_command_test.go
@@ -62,6 +62,30 @@ func TestMarshalCommand(t *testing.T) {
 				RequestType: "DisableRemoteDesktop",
 			},
 		},
+		{
+			Command: Command{
+				RequestType: "SetFirmwarePassword",
+				SetFirmwarePassword: &SetFirmwarePassword{
+					CurrentPassword: "test",
+				},
+			},
+		},
+		{
+			Command: Command{
+				RequestType: "SetFirmwarePassword",
+				SetFirmwarePassword: &SetFirmwarePassword{
+					NewPassword: "test",
+				},
+			},
+		},
+		{
+			Command: Command{
+				RequestType: "SetFirmwarePassword",
+				VerifyFirmwarePassword: &VerifyFirmwarePassword{
+					Password: "test",
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.Command.RequestType+"_json", func(t *testing.T) {
@@ -223,6 +247,60 @@ func TestEndToEnd(t *testing.T) {
 				}
 			},
 		},
+
+		{
+			name: "SetFirmwarePassword_NoNewPassword",
+			requestBytes: []byte(
+				`{"request_type":"SetFirmwarePassword","current_password":"test"}`,
+			),
+			testFn: func(t *testing.T, parts endToEndParts) {
+				needToSee := [][]byte{
+					[]byte(`CurrentPassword`),
+					[]byte(`test`),
+					[]byte(`NewPassword`),
+				}
+				for _, b := range needToSee {
+					if !bytes.Contains(parts.plistData, b) {
+						t.Error(fmt.Sprintf("marshaled plist does not contain required bytes: '%s'", string(b)))
+					}
+				}
+			},
+		},
+		{
+			name: "SetFirmwarePassword_NewPassword",
+			requestBytes: []byte(
+				`{"request_type":"SetFirmwarePassword","new_password":"test"}`,
+			),
+			testFn: func(t *testing.T, parts endToEndParts) {
+				needToSee := [][]byte{
+					[]byte(`NewPassword`),
+					[]byte(`test`),
+				}
+				for _, b := range needToSee {
+					if !bytes.Contains(parts.plistData, b) {
+						t.Error(fmt.Sprintf("marshaled plist does not contain required bytes: '%s'", string(b)))
+					}
+				}
+			},
+		},
+
+		{
+			name: "VerifyFirmwarePassword",
+			requestBytes: []byte(
+				`{"request_type":"VerifyFirmwarePassword","password":"test"}`,
+			),
+			testFn: func(t *testing.T, parts endToEndParts) {
+				needToSee := [][]byte{
+					[]byte(`Password`),
+					[]byte(`test`),
+				}
+				for _, b := range needToSee {
+					if !bytes.Contains(parts.plistData, b) {
+						t.Error(fmt.Sprintf("marshaled plist does not contain required bytes: '%s'", string(b)))
+					}
+				}
+			},
+		},
 	}
 
 	for _, tt := range tests {