Repository: micromdm/micromdm
Pull request #754 (2021-05-27 04:56:07)

Allow command callers to specify Command UUID for idempotency
## What's up? 

Apple's MDM protocol allows a `CommandUUID` to be specified in commands for correlation during the handling of acknowledgement responses. 

Though it looks like an idempotency key, as far as I can tell, Apple doesn't do anything with it except passing it back to you in responses. 

Currently, MicroMDM generates a UUID when sending commands to Apple and returns it in the payload. 

### What this PR does 
This allows the the UUID to be specified by a API caller. e.g. in a `DeviceLock` call.

```json
{
    "request_type": "DeviceLock", 
    "pin": "123456", 
    "udid": "udid-of-the-device" ,
    "command_uuid": "This-is-my-UUID" 
}
```

When a `CommandRequest` is received, we accept the `command_uuid` set in the JSON payload or use the default generated one if it's not present. 

The intent for implementing it this way is to upstream the setting of the UUID as early as we can, so the rest of the code can always assume that the `CommandRequest.CommandUUID` is available, instead of making it optional. 

### Why do this? 
To ensure idempotency, my app has a command queue. Currently it sends a command to MicroMDM, takes the `command_uuid` and write a queue item that gets fulfilled by the ACK webhook event. But this is not safe because calling the MicroMDM API can fail or flake. 

It would make the command queue more robust to allow a stable command UUID to be written before the network calls to MicroMDM are made. For example, by allowing the caller to specify the key, even if a command succeeds but the response from MicroMDM is lost, the Apple webhook will still arrive with the right command UUID set. 

### Caveats 

After doing some testing, the main thing that nags me is that `""` is a valid value for a UUID. If you submit a JSON payload with `"command_uuid": ""`, we get back: 

```json
{
    "payload": {
        "command_uuid": "",
        "command": {
            "request_type": "DeviceLock",
            "pin": "123456"
        }
    }
}
```
This is because `encoding/json` treats `""` rightfully as a valid value and doesn't apply the `uuid.New.String()` default. 

The `DeviceLock` command works and I get back the ACK webhook, the `command_uuid` is **missing** and the plist payload has: 

```
<key>CommandUUID</key>
<string></string>
```

I'm happy to dive in a little deeper to use the default if the supplied value is an empty string or a whitespace only string

P.S. Also my first time writing Go code :smiley_cat:

jessepeterson: Dup of #672 I think. :)
daemonsy: Mmm looks pretty similar, #672 looks like it's abandoned though?
jessepeterson: In general I'm in favor of this. I added some notes. Welcome to Go! It's pretty standard for Go to require code formatting. This looks to be why the checks are failing — you ought to [`gofmt` your code (or better yet have your editor do it for you)](https://blog.golang.org/gofmt). :)
daemonsy: Hey @jessepeterson took a stab at your comments, also used gofmt. At least locally, seems like the command used by CI returns 0 line differences. 

I realized there's tests and wanted to add one, but it seems like the most relevant one is `mdm_command_test.go`. I sneaked a test into the `InstallProfile` case, which feels like a hijack. If you have advice on making a test case that is clearer, happy to do it. 
jessepeterson: @daemonsy this looks good. If you want to add a different test and then add a line item describing this change in the `CHANGELOG` for unreleased then we can get it merged!
daemonsy: Hey @jessepeterson Just got back to the computer world. I'm trying to add it as a separate test case. Will add the Changelog item too. Also, do you want me to rebase it into a single commit? Happy for you to squash merge the PR too. 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a930ff3..b3756d9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,7 @@
 ## [Unreleased](https://github.com/micromdm/micromdm/compare/v1.8.0...main) TBD
 
 - Fix SetFirmwarePassword and VerifyFirmwarePassword parameters (#743)
+- Command UUID can now be passed in as as a request parameter (#754)
 
 ## [v1.8.0](https://github.com/micromdm/micromdm/compare/v1.7.1...v1.8.0) February, 2021
 
diff --git a/mdm/mdm/command.go b/mdm/mdm/command.go
index 16e2b81..4627387 100644
--- a/mdm/mdm/command.go
+++ b/mdm/mdm/command.go
@@ -1,12 +1,15 @@
 package mdm
 
 import (
+	"strings"
+
 	"github.com/google/uuid"
 	"github.com/micromdm/micromdm/mdm/appmanifest"
 )
 
 type CommandRequest struct {
-	UDID string `json:"udid"`
+	UDID        string `json:"udid"`
+	CommandUUID string `json:"command_uuid"`
 	*Command
 }
 
@@ -17,9 +20,12 @@ type CommandPayload struct {
 
 func NewCommandPayload(request *CommandRequest) (*CommandPayload, error) {
 	payload := &CommandPayload{
-		CommandUUID: uuid.New().String(),
+		CommandUUID: request.CommandUUID,
 		Command:     request.Command,
 	}
+	if strings.TrimSpace(payload.CommandUUID) == "" {
+		payload.CommandUUID = uuid.New().String()
+	}
 	return payload, nil
 }
 
diff --git a/mdm/mdm/unmarshal_json.go b/mdm/mdm/unmarshal_json.go
index 5324672..6622d55 100644
--- a/mdm/mdm/unmarshal_json.go
+++ b/mdm/mdm/unmarshal_json.go
@@ -11,12 +11,14 @@ func (c *CommandRequest) UnmarshalJSON(data []byte) error {
 	var request = struct {
 		UDID        string `json:"udid"`
 		RequestType string `json:"request_type"`
+		CommandUUID string `json:"command_uuid"`
 	}{}
 	if err := json.Unmarshal(data, &request); err != nil {
 		return errors.Wrap(err, "mdm: unmarshal json command request")
 	}
 	c.UDID = request.UDID
 	c.Command = &Command{}
+	c.CommandUUID = request.CommandUUID
 	return c.Command.UnmarshalJSON(data)
 }
 


Test patch:
diff --git a/mdm/mdm/mdm_command_test.go b/mdm/mdm/mdm_command_test.go
index 84a3efb..cdb426e 100644
--- a/mdm/mdm/mdm_command_test.go
+++ b/mdm/mdm/mdm_command_test.go
@@ -133,6 +133,49 @@ func TestUnmarshalCommandPayload(t *testing.T) {
 		})
 	}
 }
+func TestNewCommandPayload(t *testing.T) {
+	// Unit test cases for request params
+	var tests = []struct {
+		name    string
+		request CommandRequest
+		testFn  func(t *testing.T, payload *CommandPayload)
+	}{
+		{
+			name:    "Uses UUID passed to CommandRequest",
+			request: CommandRequest{CommandUUID: "this-uuid-should-be-used"},
+			testFn: func(t *testing.T, payload *CommandPayload) {
+				if payload.CommandUUID != "this-uuid-should-be-used" {
+					t.Error("CommandUUID is not set to CommandRequest.CommandUUID")
+				}
+			},
+		},
+		{
+			name:    "Defaults to generated UUID if CommandUUID is an empty string",
+			request: CommandRequest{CommandUUID: ""},
+			testFn: func(t *testing.T, payload *CommandPayload) {
+				if payload.CommandUUID == "" {
+					t.Error("CommandUUID should be a generated UUID")
+				}
+			},
+		},
+		{
+			name:    "Defaults to generated UUID if CommandUUID is all whitespace",
+			request: CommandRequest{CommandUUID: " "},
+			testFn: func(t *testing.T, payload *CommandPayload) {
+				if payload.CommandUUID == " " {
+					t.Error("CommandUUID should be a generated UUID")
+				}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var payload, _ = NewCommandPayload(&tt.request)
+			tt.testFn(t, payload)
+		})
+	}
+}
 
 func mustLoadFile(t *testing.T, filename string) []byte {
 	t.Helper()