From 14fbe40d3ee80f534e47516894b6fdf923cda4b4 Mon Sep 17 00:00:00 2001 From: Neil Martin Date: Thu, 1 Oct 2026 20:57:57 +0100 Subject: [PATCH 1/4] fix(scope)!: resolve --computer/--mobile-device to an ID and send alone A Mac's UDID never reached the wire as a UDID. udidRe matched only the 40-hex shape older iOS devices use, so a UUID-shaped Mac UDID went out as ; and once matched, NamedItem's untagged Name still sent an empty beside the . The Classic computer matcher reads , , in order and refuses at the first one present that does not match, so both forms answered 409 "Unable to match computer" on every computer-scoped resource. The mobile-device matcher accepts any matching element, which is why mobile UDIDs worked. Wire-checked 2026-10-01 on platform-nmjspp: each of , , and resolves alone, case-insensitively; a stale or an empty ahead of a good identifier fails. Names are not unique and the server's name match picks one duplicate silently. --computer and --mobile-device now take an ID, name, UDID or serial number, resolved in one inventory request (/v4/computers-inventory, /v2/mobile-devices/detail, both published on the gateway) to the device's ID, which is the only identifier sent. add's duplicate check, remove and the read-back verification compare IDs, so remove by serial number now works (the scope GET carries no serial). remove falls back to the literal value only when nothing matches, so a member whose inventory record is gone can still be taken out. Also: parseMobileDevice reads general.udid and hardware.serialNumber, which is where /v2/mobile-devices/detail puts them. BREAKING CHANGE: a device name shared by more than one device is refused instead of letting the server pick one; pass the ID. The resolution needs Read Computers or Read Mobile Devices on the API client. Co-Authored-By: Claude Opus 5.5 --- internal/resolve/identifier.go | 156 ++++++++++++++++++++++++++++ internal/resolve/identifier_test.go | 127 ++++++++++++++++++++++ internal/resolve/resolve.go | 8 ++ internal/scope/commands.go | 96 ++++++++++++++--- internal/scope/scope.go | 13 ++- internal/scope/scope_test.go | 150 +++++++++++++++++++++++++- internal/scope/types.go | 32 +++++- internal/scope/verify.go | 2 +- 8 files changed, 559 insertions(+), 25 deletions(-) create mode 100644 internal/resolve/identifier.go create mode 100644 internal/resolve/identifier_test.go diff --git a/internal/resolve/identifier.go b/internal/resolve/identifier.go new file mode 100644 index 00000000..3dd9ee3d --- /dev/null +++ b/internal/resolve/identifier.go @@ -0,0 +1,156 @@ +// Copyright 2026, Jamf Software LLC + +package resolve + +import ( + "context" + "errors" + "fmt" + "net/url" + "strings" + + "github.com/Jamf-Concepts/jamf-cli/internal/registry" +) + +// ErrNoDeviceMatch is what a NoDeviceMatchError answers errors.Is with, so a +// caller can tell "nothing is called that" from a failed lookup. +var ErrNoDeviceMatch = errors.New("no matching device") + +// NoDeviceMatchError reports a value that names no device. +type NoDeviceMatchError struct { + Label string // "computer" / "mobile device" + Value string +} + +func (e *NoDeviceMatchError) Error() string { + return fmt.Sprintf("no %s found with ID, name, UDID or serial number %q", e.Label, e.Value) +} + +// Is makes errors.Is(err, ErrNoDeviceMatch) hold. +func (e *NoDeviceMatchError) Is(target error) bool { return target == ErrNoDeviceMatch } + +// identifierSpec describes one device family's inventory endpoint for +// ResolveComputerIdentifier / ResolveMobileDeviceIdentifier. +type identifierSpec struct { + label string // "computer" / "mobile device" + path string // inventory list path, with the sections parse needs + idField string // RSQL field holding the numeric ID + // fields are the RSQL fields holding the name, UDID and serial number. + nameField, udidField, serialField string + parse func(map[string]any) (*DeviceIdentifiers, error) +} + +// Both endpoints are published on the platform gateway; the per-ID inventory +// paths the other resolvers use are not all, so the ID goes through the filter +// as well. +var computerIdentifierSpec = identifierSpec{ + label: "computer", + path: "/v4/computers-inventory?section=GENERAL§ion=HARDWARE", + idField: "id", + nameField: "general.name", + udidField: "udid", + serialField: "hardware.serialNumber", + parse: parseComputerInventory, +} + +var mobileIdentifierSpec = identifierSpec{ + label: "mobile device", + path: "/v2/mobile-devices/detail?section=GENERAL§ion=HARDWARE", + idField: "mobileDeviceId", + nameField: "displayName", + udidField: "udid", + serialField: "serialNumber", + parse: parseMobileDevice, +} + +// identifierPageSize bounds the lookup. One exact identifier names a handful of +// records at most (names are not unique); anything past this is reported as +// ambiguous all the same. +const identifierPageSize = 20 + +// ResolveComputerIdentifier resolves a value that may be a computer's Jamf Pro +// ID, name, UDID or serial number to one computer, in a single request. +// +// See resolveIdentifier for how a value matching more than one record is +// handled. +func ResolveComputerIdentifier(ctx context.Context, client registry.HTTPClient, value string) (*DeviceIdentifiers, error) { + return resolveIdentifier(ctx, client, computerIdentifierSpec, value) +} + +// ResolveMobileDeviceIdentifier is ResolveComputerIdentifier for mobile devices. +func ResolveMobileDeviceIdentifier(ctx context.Context, client registry.HTTPClient, value string) (*DeviceIdentifiers, error) { + return resolveIdentifier(ctx, client, mobileIdentifierSpec, value) +} + +// resolveIdentifier ORs the value across every identifier field and then +// re-checks each result exactly, since RSQL `==` treats `*` as a wildcard. +// +// A numeric value that is some record's ID resolves to that record even when +// another record carries the same digits as its name, because the numeric-ID +// reading is the CLI's documented contract and the one the Classic API itself +// applies first. Any other value must name exactly one record: Classic names +// are not unique, and the Classic API's own name matching would silently pick +// one of the duplicates. +func resolveIdentifier(ctx context.Context, client registry.HTTPClient, spec identifierSpec, value string) (*DeviceIdentifiers, error) { + value = strings.TrimSpace(value) + if value == "" { + return nil, fmt.Errorf("empty %s identifier", spec.label) + } + + quoted := `"` + EscapeRSQL(value) + `"` + clauses := []string{ + spec.nameField + "==" + quoted, + spec.udidField + "==" + quoted, + spec.serialField + "==" + quoted, + } + if isNumericID(value) { + clauses = append([]string{spec.idField + "==" + value}, clauses...) + } + path := fmt.Sprintf("%s&page-size=%d&filter=%s", spec.path, identifierPageSize, + url.QueryEscape(strings.Join(clauses, ","))) + + records, _, err := fetchInventoryPage(ctx, client, path) + if err != nil { + return nil, fmt.Errorf("looking up %s %q: %w", spec.label, value, err) + } + + var matches []*DeviceIdentifiers + for _, record := range records { + d, err := spec.parse(record) + if err != nil { + continue + } + if isNumericID(value) && d.ID == value { + return d, nil + } + if matchedBy(d, value) != "" { + matches = append(matches, d) + } + } + + switch len(matches) { + case 0: + return nil, &NoDeviceMatchError{Label: spec.label, Value: value} + case 1: + return matches[0], nil + } + described := make([]string, len(matches)) + for i, d := range matches { + described[i] = fmt.Sprintf("id %s (%s)", d.ID, matchedBy(d, value)) + } + return nil, fmt.Errorf("%q matches %d %ss: %s; pass the numeric ID of the one you mean", + value, len(matches), spec.label, strings.Join(described, ", ")) +} + +// matchedBy names the identifier a record matched value on, or "" for none. +func matchedBy(d *DeviceIdentifiers, value string) string { + switch { + case strings.EqualFold(d.Name, value): + return "name" + case strings.EqualFold(d.UDID, value): + return "UDID" + case strings.EqualFold(d.SerialNumber, value): + return "serial number" + } + return "" +} diff --git a/internal/resolve/identifier_test.go b/internal/resolve/identifier_test.go new file mode 100644 index 00000000..71b0cbb9 --- /dev/null +++ b/internal/resolve/identifier_test.go @@ -0,0 +1,127 @@ +// Copyright 2026, Jamf Software LLC + +package resolve + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" +) + +func inventoryPage(records ...string) string { + return fmt.Sprintf(`{"totalCount":%d,"results":[%s]}`, len(records), strings.Join(records, ",")) +} + +// mobileDetailRecord is the /v2/mobile-devices/detail shape as the wire sends +// it with section=GENERAL§ion=HARDWARE: udid and displayName under +// general, serialNumber under hardware, nothing at the top level but the id. +func mobileDetailRecord(id, name, udid, serial string) string { + return fmt.Sprintf(`{"mobileDeviceId":%q,"general":{"displayName":%q,"udid":%q,"managementId":"m-%s"},"hardware":{"serialNumber":%q}}`, + id, name, udid, id, serial) +} + +func TestResolveComputerIdentifier_EachIdentifierKind(t *testing.T) { + rec := `{"id":"107","udid":"96050be1-e53b-454d-9752-2306c709f192","general":{"name":"ARMADA-058JG5"},"hardware":{"serialNumber":"FWWT058JG5"}}` + for _, value := range []string{"107", "armada-058jg5", "96050BE1-E53B-454D-9752-2306C709F192", "fwwt058jg5"} { + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(rec)}}} + d, err := ResolveComputerIdentifier(context.Background(), client, value) + if err != nil { + t.Fatalf("%s: %v", value, err) + } + if d.ID != "107" { + t.Errorf("%s: ID = %q, want 107", value, d.ID) + } + if len(client.calls) != 1 { + t.Errorf("%s: want one request, got %v", value, client.calls) + } + } +} + +func TestResolveComputerIdentifier_FilterORsEveryField(t *testing.T) { + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(computerRecord("7"))}}} + if _, err := ResolveComputerIdentifier(context.Background(), client, "7"); err != nil { + t.Fatal(err) + } + want := `filter=id==7,general.name=="7",udid=="7",hardware.serialNumber=="7"` + if !strings.Contains(client.calls[0], want) { + t.Errorf("request %q does not carry %s", client.calls[0], want) + } + + client = &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(computerRecord("7"))}}} + _, _ = ResolveComputerIdentifier(context.Background(), client, "SER7") + if strings.Contains(client.calls[0], "filter=id==") { + t.Errorf("a non-numeric value must not be compared against the numeric id field: %s", client.calls[0]) + } +} + +// A numeric value that is a record's ID wins over another record that carries +// the same digits as its name: the numeric reading is the documented contract. +func TestResolveComputerIdentifier_IDWinsOverNumericName(t *testing.T) { + named := `{"id":"9","udid":"u9","general":{"name":"42"},"hardware":{"serialNumber":"S9"}}` + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(named, computerRecord("42"))}}} + d, err := ResolveComputerIdentifier(context.Background(), client, "42") + if err != nil { + t.Fatal(err) + } + if d.ID != "42" { + t.Errorf("ID = %q, want 42", d.ID) + } +} + +func TestResolveComputerIdentifier_DuplicateNameIsRefused(t *testing.T) { + a := `{"id":"4","udid":"u4","general":{"name":"FVFZCAK0LYWH"},"hardware":{"serialNumber":"FVFZCAK0LYWH"}}` + b := `{"id":"31","udid":"u31","general":{"name":"FVFZCAK0LYWH"},"hardware":{"serialNumber":"OTHER"}}` + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(a, b)}}} + _, err := ResolveComputerIdentifier(context.Background(), client, "FVFZCAK0LYWH") + if err == nil { + t.Fatal("expected an ambiguity error") + } + for _, want := range []string{"id 4", "id 31", "numeric ID"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + if errors.Is(err, ErrNoDeviceMatch) { + t.Error("an ambiguous value is not a missing one") + } +} + +// RSQL == treats * as a wildcard, so a server-side match is re-checked exactly. +func TestResolveComputerIdentifier_WildcardMatchIsNotExact(t *testing.T) { + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage(computerRecord("1"), computerRecord("2"))}}} + _, err := ResolveComputerIdentifier(context.Background(), client, "mac-*") + if !errors.Is(err, ErrNoDeviceMatch) { + t.Fatalf("want ErrNoDeviceMatch, got %v", err) + } +} + +func TestResolveComputerIdentifier_NotFoundAndLookupFailure(t *testing.T) { + client := &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {200, inventoryPage()}}} + if _, err := ResolveComputerIdentifier(context.Background(), client, "nope"); !errors.Is(err, ErrNoDeviceMatch) { + t.Errorf("want ErrNoDeviceMatch, got %v", err) + } + client = &mockClient{responses: map[string]mockResponse{"v4/computers-inventory?": {403, `{}`}}} + _, err := ResolveComputerIdentifier(context.Background(), client, "nope") + if err == nil || errors.Is(err, ErrNoDeviceMatch) { + t.Errorf("a failed lookup must not read as no match: %v", err) + } +} + +func TestResolveMobileDeviceIdentifier_DetailShape(t *testing.T) { + rec := mobileDetailRecord("64", "ARMADA-66B185", "5f3644dc5303edf83f28a8d6070c1e955ba91af1", "GMJR66B185") + for _, value := range []string{"64", "armada-66b185", "5F3644DC5303EDF83F28A8D6070C1E955BA91AF1", "gmjr66b185"} { + client := &mockClient{responses: map[string]mockResponse{"v2/mobile-devices/detail?": {200, inventoryPage(rec)}}} + d, err := ResolveMobileDeviceIdentifier(context.Background(), client, value) + if err != nil { + t.Fatalf("%s: %v", value, err) + } + if d.ID != "64" || d.UDID == "" || d.SerialNumber == "" { + t.Errorf("%s: got %+v", value, d) + } + if !strings.Contains(client.calls[0], "section=HARDWARE") { + t.Errorf("%s: the serial is only populated with section=HARDWARE: %s", value, client.calls[0]) + } + } +} diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index c59fb61e..41825a68 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -460,6 +460,14 @@ func parseMobileDevice(obj map[string]any) (*DeviceIdentifiers, error) { if d.Name == "" { d.Name = jsonString(general, "displayName") } + if d.UDID == "" { + d.UDID = jsonString(general, "udid") + } + } + // /detail nests the serial under "hardware", populated only with + // section=HARDWARE. + if hardware, ok := obj["hardware"].(map[string]any); ok && d.SerialNumber == "" { + d.SerialNumber = jsonString(hardware, "serialNumber") } if d.Name == "" { d.Name = jsonString(obj, "displayName") diff --git a/internal/scope/commands.go b/internal/scope/commands.go index b4cad8fc..e4f4932d 100644 --- a/internal/scope/commands.go +++ b/internal/scope/commands.go @@ -3,6 +3,8 @@ package scope import ( + "context" + "errors" "fmt" "os" "strings" @@ -10,6 +12,7 @@ import ( "github.com/spf13/cobra" "github.com/Jamf-Concepts/jamf-cli/internal/registry" + "github.com/Jamf-Concepts/jamf-cli/internal/resolve" ) // NewScopeCmd creates the "scope" subcommand group with get, add, and remove @@ -104,6 +107,9 @@ func newScopeAddCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { if err := ValidateScopeCombination(res.SingularKey, section, target.FlagName); err != nil { return err } + if target, err = resolveDeviceTarget(cmd.Context(), ctx.Client, target, false); err != nil { + return err + } id, s, err := FetchScope(cmd.Context(), ctx.Client, res, ref) if err != nil { @@ -115,8 +121,8 @@ func newScopeAddCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { } if !AddToScope(s, section, target.FlagName, target.Name) { - fmt.Fprintf(os.Stderr, "%s %q already in %s scope of %s\n", - target.FlagName, target.Name, section, ref) + fmt.Fprintf(os.Stderr, "%s %s already in %s scope of %s\n", + target.FlagName, target.display(), section, ref) return nil } @@ -132,8 +138,8 @@ func newScopeAddCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { return err } - fmt.Fprintf(os.Stderr, "Added %s %q to %s scope of %s\n", - target.FlagName, target.Name, section, ref) + fmt.Fprintf(os.Stderr, "Added %s %s to %s scope of %s\n", + target.FlagName, target.display(), section, ref) return OutputScope(ctx.Output, s, outputFormat(cmd)) }, } @@ -164,6 +170,9 @@ func newScopeRemoveCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { if err := ValidateScopeCombination(res.SingularKey, section, target.FlagName); err != nil { return err } + if target, err = resolveDeviceTarget(cmd.Context(), ctx.Client, target, true); err != nil { + return err + } id, s, err := FetchScope(cmd.Context(), ctx.Client, res, ref) if err != nil { @@ -171,8 +180,8 @@ func newScopeRemoveCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { } if !RemoveFromScope(s, section, target.FlagName, target.Name) { - fmt.Fprintf(os.Stderr, "%s %q not found in %s scope of %s\n", - target.FlagName, target.Name, section, ref) + fmt.Fprintf(os.Stderr, "%s %s not found in %s scope of %s\n", + target.FlagName, target.display(), section, ref) return nil } @@ -184,8 +193,8 @@ func newScopeRemoveCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { return err } - fmt.Fprintf(os.Stderr, "Removed %s %q from %s scope of %s\n", - target.FlagName, target.Name, section, ref) + fmt.Fprintf(os.Stderr, "Removed %s %s from %s scope of %s\n", + target.FlagName, target.display(), section, ref) return OutputScope(ctx.Output, s, outputFormat(cmd)) }, } @@ -195,6 +204,50 @@ func newScopeRemoveCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { return cmd } +// resolveDevice is swapped by tests. +var resolveDevice = func(ctx context.Context, client registry.HTTPClient, flagName, value string) (*resolve.DeviceIdentifiers, error) { + if flagName == flagComputer { + return resolve.ResolveComputerIdentifier(ctx, client, value) + } + return resolve.ResolveMobileDeviceIdentifier(ctx, client, value) +} + +// resolveDeviceTarget turns a --computer or --mobile-device value — an ID, +// name, UDID or serial number — into the device's numeric ID, which is then +// the only identifier sent. +// +// alone is the one form both Classic matchers resolve unconditionally. +// Wire-checked 2026-10-01 on a policy and a mobile profile, targets and +// exclusions: each of , , and resolves on its +// own, but the computer matcher reads them in order and refuses at the first +// one present that does not match — an empty before a good , or a +// stale before a good , answers 409 "Unable to match computer". A +// name is also not unique, and the server's own name match picks one duplicate +// silently, which the resolver refuses instead. Resolving first also makes +// add's duplicate check, remove's lookup and the verification read compare +// IDs, where a serial number could not be compared at all: the scope GET +// carries no serial. +// +// remove falls back to the literal value when the device no longer resolves, +// so a member whose inventory record is gone can still be taken out by the ID +// or name the scope lists it under. +func resolveDeviceTarget(ctx context.Context, client registry.HTTPClient, target ScopeTarget, forRemove bool) (ScopeTarget, error) { + if !isDeviceFlag(target.FlagName) { + return target, nil + } + target.Input = target.Name + d, err := resolveDevice(ctx, client, target.FlagName, target.Name) + if err != nil { + if forRemove && errors.Is(err, resolve.ErrNoDeviceMatch) { + return target, nil + } + return target, fmt.Errorf("--%s: %w", target.FlagName, err) + } + target.Device = d + target.Name = d.ID + return target, nil +} + // verifyWritten re-reads the scope and confirms the write landed, unless this // is a dry run. // @@ -215,7 +268,8 @@ func verifyWritten(cmd *cobra.Command, ctx *registry.CLIContext, res Resource, i // and categories THIS resource accepts rather than the union across all eight. func mutateLong(res Resource, verb string) string { sections := SectionsFor(res.SingularKey) - return fmt.Sprintf(`%s the scope of %s. + flags := ScopeFlagsFor(res.SingularKey) + long := fmt.Sprintf(`%s the scope of %s. Sections (--section): %s. Default: %s. Categories: %s. @@ -223,7 +277,23 @@ Categories: %s. Only the element is sent, so nothing else about the object is rewritten. The scope itself is replaced whole, which is why the current scope is read first.`, - verb, describe(res), humanList(sections), sections[0], flagList(ScopeFlagsFor(res.SingularKey))) + verb, describe(res), humanList(sections), sections[0], flagList(flags)) + + var devices []string + for _, f := range flags { + if isDeviceFlag(f) { + devices = append(devices, "--"+f) + } + } + if len(devices) == 0 { + return long + } + return long + fmt.Sprintf(` + +A device (%s) can be given by ID, name, UDID or serial +number. It is resolved against inventory to its ID before anything is written, +so the API client also needs Read Computers or Read Mobile Devices. A name +shared by more than one device is refused; pass the ID instead.`, humanList(devices)) } // scopeExample renders examples using categories the resource really has, so @@ -361,12 +431,12 @@ func scopeFlagHelp(singularKey, flag string) string { // scopeFlagNoun describes what each flag's value identifies. The device and // directory entries carry the detail a caller cannot guess: which flags accept -// an ID or UDID as well as a name, and which two are free text resolved +// an ID, UDID or serial number as well as a name, and which two are free text resolved // against the directory rather than Jamf Pro object names. var scopeFlagNoun = map[string]string{ - flagComputer: "individual computer (id, name, or UDID)", + flagComputer: "individual computer (id, name, UDID or serial number)", flagComputerGroup: "computer group name", - flagMobileDevice: "individual mobile device (id, name, or UDID)", + flagMobileDevice: "individual mobile device (id, name, UDID or serial number)", flagMobileDeviceGroup: "mobile device group name", flagBuilding: "building name", flagDepartment: "department name", diff --git a/internal/scope/scope.go b/internal/scope/scope.go index be9947a7..1f9a57a6 100644 --- a/internal/scope/scope.go +++ b/internal/scope/scope.go @@ -19,14 +19,17 @@ import ( "github.com/Jamf-Concepts/jamf-cli/internal/registry" ) -// udidRe matches a 40-character hex string — the format of Apple device UDIDs. -var udidRe = regexp.MustCompile(`^[0-9a-fA-F]{40}$`) +// udidRe matches the three shapes an Apple device UDID takes: 40 hex characters +// (older iOS devices), 8-16 hex (iOS devices from 2018 on) and a UUID (every +// Mac). Matching only the first sent a Mac's UDID as , which the Classic +// API answers with 409 "Unable to match computer". +var udidRe = regexp.MustCompile(`^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{8}-[0-9a-fA-F]{16}|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})$`) // numericRe matches a plain integer string (Jamf Pro Classic API numeric ID). var numericRe = regexp.MustCompile(`^[0-9]+$`) // namedItemFromIdentifier builds a NamedItem with the correct field populated -// based on what the caller passed: a 40-char hex UDID, a numeric ID, or a name. +// based on what the caller passed: a UDID, a numeric ID, or a name. // Used for individual device scope targets where the API accepts any of the three. func namedItemFromIdentifier(value string) NamedItem { switch { @@ -317,10 +320,10 @@ func marshalScopeBody(singularKey string, s *ScopeXML) ([]byte, error) { func silentDropError(singularKey, section, flagName, itemName string, expectedPresent bool) error { if expectedPresent { - return fmt.Errorf("the server accepted the write but %s %q is not in the %s scope of this %s; the likeliest cause is that the identifier names no existing record", + return fmt.Errorf("the server accepted the write but %s %s is not in the %s scope of this %s; the likeliest cause is that the identifier names no existing record", flagName, itemName, section, singularKey) } - return fmt.Errorf("the server accepted the write but %s %q is still in the %s scope of this %s", + return fmt.Errorf("the server accepted the write but %s %s is still in the %s scope of this %s", flagName, itemName, section, singularKey) } diff --git a/internal/scope/scope_test.go b/internal/scope/scope_test.go index 5dcff95a..aaf5c76d 100644 --- a/internal/scope/scope_test.go +++ b/internal/scope/scope_test.go @@ -6,12 +6,14 @@ import ( "context" "encoding/json" "encoding/xml" + "errors" "io" "net/http" "strings" "testing" "github.com/Jamf-Concepts/jamf-cli/internal/registry" + "github.com/Jamf-Concepts/jamf-cli/internal/resolve" ) // ─── XML round-trip ──────────────────────────────────────────────────────────── @@ -501,12 +503,32 @@ func TestValidateScopeCombination_ValidExclusions(t *testing.T) { // ─── namedItemFromIdentifier ───────────────────────────────────────────────── func TestNamedItemFromIdentifier_UDID(t *testing.T) { - item := namedItemFromIdentifier("270aae10800b6e61a2ee2bbc285eb967050b5984") - if item.UDID != "270aae10800b6e61a2ee2bbc285eb967050b5984" { - t.Errorf("UDID = %q", item.UDID) + for _, udid := range []string{ + "270aae10800b6e61a2ee2bbc285eb967050b5984", // older iOS device + "00008030-001A2D3E0C38802E", // iOS device from 2018 on + "96050be1-e53b-454d-9752-2306c709f192", // Mac (UUID) + "96050BE1-E53B-454D-9752-2306C709F192", + } { + item := namedItemFromIdentifier(udid) + if item.UDID != udid { + t.Errorf("%s: UDID = %q", udid, item.UDID) + } + if item.Name != "" || item.ID != "" { + t.Errorf("%s: unexpected fields set: name=%q id=%q", udid, item.Name, item.ID) + } } - if item.Name != "" || item.ID != "" { - t.Errorf("unexpected fields set: name=%q id=%q", item.Name, item.ID) +} + +func TestNamedItemFromIdentifier_NearUDIDIsAName(t *testing.T) { + for _, name := range []string{ + "96050be1-e53b-454d-9752-2306c709f19", // UUID one short + "96050be1e53b454d97522306c709f192", // UUID without hyphens + "00008030-001A2D3E0C38802", // 8-16 one short + "Lab-Mac-96050be1-e53b-454d-9752-2306", // hyphenated name + } { + if item := namedItemFromIdentifier(name); item.Name != name { + t.Errorf("%s: want Name, got %+v", name, item) + } } } @@ -1236,3 +1258,121 @@ func TestMarshalScopeBody_FieldOrderIsSchemaOrder(t *testing.T) { t.Errorf("body should open with the XML declaration:\n%s", got) } } + +// A UDID-identified member must reach the wire as alone. Wire-checked +// 2026-10-01 on all five computer-scoped resources, target and exclusion: +// … answers 409 "Unable to match computer" because +// the empty name is matched first, while … alone resolves. +func TestMarshalScopeBody_UDIDMemberCarriesNoEmptyName(t *testing.T) { + s := &ScopeXML{} + const udid = "96050be1-e53b-454d-9752-2306c709f192" + if !AddToScope(s, "target", "computer", udid) { + t.Fatal("AddToScope returned false") + } + if !AddToScope(s, "exclusion", "computer", strings.ToUpper(udid)) { + t.Fatal("AddToScope (exclusion) returned false") + } + body, err := marshalScopeBody("mac_application", s) + if err != nil { + t.Fatalf("marshalScopeBody: %v", err) + } + got := string(body) + if strings.Count(got, "") != 2 { + t.Errorf("want two elements:\n%s", got) + } + if strings.Contains(got, "") || strings.Contains(got, "") { + t.Errorf("a UDID-identified member must not carry an empty :\n%s", got) + } +} + +// ─── resolveDeviceTarget ───────────────────────────────────────────────────── + +func stubResolveDevice(t *testing.T, fn func(flag, value string) (*resolve.DeviceIdentifiers, error)) { + t.Helper() + orig := resolveDevice + resolveDevice = func(_ context.Context, _ registry.HTTPClient, flag, value string) (*resolve.DeviceIdentifiers, error) { + return fn(flag, value) + } + t.Cleanup(func() { resolveDevice = orig }) +} + +func TestResolveDeviceTarget_SendsOnlyTheResolvedID(t *testing.T) { + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + return &resolve.DeviceIdentifiers{ID: "107", Name: "ARMADA-058JG5", UDID: "96050be1-e53b-454d-9752-2306c709f192", SerialNumber: "FWWT058JG5"}, nil + }) + target, err := resolveDeviceTarget(context.Background(), nil, ScopeTarget{FlagName: "computer", Name: "FWWT058JG5"}, false) + if err != nil { + t.Fatal(err) + } + if target.Name != "107" || target.Input != "FWWT058JG5" { + t.Fatalf("got %+v", target) + } + + s := &ScopeXML{} + if !AddToScope(s, "exclusion", target.FlagName, target.Name) { + t.Fatal("AddToScope returned false") + } + body, err := marshalScopeBody("policy", s) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(body), "\n") || strings.Contains(string(body), "") || strings.Contains(string(body), "") { + t.Errorf("want alone:\n%s", body) + } + if !strings.Contains(string(body), "107") { + t.Errorf("missing 107:\n%s", body) + } + if got := target.display(); got != `"FWWT058JG5" (id 107, ARMADA-058JG5)` { + t.Errorf("display = %s", got) + } +} + +// A serial number is not in the scope GET, so remove could only find the +// member through its resolved ID. +func TestResolveDeviceTarget_RemoveBySerialFindsTheMember(t *testing.T) { + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + return &resolve.DeviceIdentifiers{ID: "64", Name: "ARMADA-66B185"}, nil + }) + s := &ScopeXML{} + s.MobileDevices.Items = []NamedItem{{ID: "64", Name: "ARMADA-66B185", UDID: "5f3644dc"}} + target, err := resolveDeviceTarget(context.Background(), nil, ScopeTarget{FlagName: "mobile-device", Name: "GMJR66B185"}, true) + if err != nil { + t.Fatal(err) + } + if !RemoveFromScope(s, "target", target.FlagName, target.Name) { + t.Fatal("member not removed") + } +} + +func TestResolveDeviceTarget_RemoveFallsBackOnlyWhenNothingMatches(t *testing.T) { + stubResolveDevice(t, func(_, value string) (*resolve.DeviceIdentifiers, error) { + return nil, &resolve.NoDeviceMatchError{Label: "computer", Value: value} + }) + in := ScopeTarget{FlagName: "computer", Name: "gone-mac"} + if _, err := resolveDeviceTarget(context.Background(), nil, in, false); err == nil { + t.Error("add must refuse a value that names no device") + } + target, err := resolveDeviceTarget(context.Background(), nil, in, true) + if err != nil || target.Name != "gone-mac" || target.Device != nil { + t.Errorf("remove should fall back to the literal value: %+v, %v", target, err) + } + + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + return nil, errors.New("HTTP 403") + }) + if _, err := resolveDeviceTarget(context.Background(), nil, in, true); err == nil { + t.Error("a failed lookup must not fall back on remove") + } +} + +func TestResolveDeviceTarget_NonDeviceFlagIsUntouched(t *testing.T) { + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + t.Fatal("resolver called for a non-device flag") + return nil, nil + }) + in := ScopeTarget{FlagName: "computer-group", Name: "Lab Macs"} + got, err := resolveDeviceTarget(context.Background(), nil, in, false) + if err != nil || got != in { + t.Errorf("got %+v, %v", got, err) + } +} diff --git a/internal/scope/types.go b/internal/scope/types.go index f3c7207b..3fe56adf 100644 --- a/internal/scope/types.go +++ b/internal/scope/types.go @@ -8,7 +8,10 @@ package scope import ( "encoding/json" "encoding/xml" + "fmt" "strings" + + "github.com/Jamf-Concepts/jamf-cli/internal/resolve" ) // Resource identifies a Classic API resource that supports scope operations. @@ -27,9 +30,30 @@ type Resource struct { } // ScopeTarget holds a resolved flag name and value from a scope add/remove command. +// +// For --computer and --mobile-device, Name is the device's numeric ID once +// resolveDeviceTarget has run, and Input keeps what the caller typed for +// messages. type ScopeTarget struct { FlagName string Name string + Input string + Device *resolve.DeviceIdentifiers +} + +// display renders the target for a message: the caller's own words, plus the +// record they resolved to when that is not obvious from them. +func (t ScopeTarget) display() string { + if t.Device == nil { + return fmt.Sprintf("%q", t.Name) + } + switch { + case t.Input == t.Device.ID: + return fmt.Sprintf("%q (%s)", t.Input, t.Device.Name) + case strings.EqualFold(t.Input, t.Device.Name): + return fmt.Sprintf("%q (id %s)", t.Input, t.Device.ID) + } + return fmt.Sprintf("%q (id %s, %s)", t.Input, t.Device.ID, t.Device.Name) } // ─── XML types ───────────────────────────────────────────────────────────────── @@ -42,9 +66,15 @@ type ScopeTarget struct { // ID is a string to accommodate both integer IDs (most resources) and UUID // IDs (e.g. ebook scope user groups) returned by the Classic API. // UDID is populated for individual mobile devices and computers. +// +// Name is omitempty in XML because the Classic API's computer matcher reads an +// empty before a : a computer sent as … +// answers 409 "Unable to match computer" where alone resolves, on every +// computer-scoped resource. The mobile-device matcher tolerates the empty name, +// and an wins over it on both. Wire-checked 2026-10-01. type NamedItem struct { ID string `xml:"id,omitempty" json:"id,omitempty"` - Name string `xml:"name" json:"name"` + Name string `xml:"name,omitempty" json:"name"` UDID string `xml:"udid,omitempty" json:"udid,omitempty"` } diff --git a/internal/scope/verify.go b/internal/scope/verify.go index 0f02bd2b..fd639465 100644 --- a/internal/scope/verify.go +++ b/internal/scope/verify.go @@ -43,7 +43,7 @@ func VerifyScopeWrite(ctx context.Context, client registry.HTTPClient, res Resou // it should be there is usually an identifier that named no record, which // is a different diagnosis from a category the server refused to keep. if items := readScopeItems(got, touchedSection, touched.FlagName); itemPresent(items, touched.Name) != expectPresent { - return silentDropError(res.SingularKey, touchedSection, touched.FlagName, touched.Name, expectPresent) + return silentDropError(res.SingularKey, touchedSection, touched.FlagName, touched.display(), expectPresent) } drops := DiffScope(sent, got, touchedSection, touched) From 7b6edb98b31383243c171e4986bca49c91bf93e0 Mon Sep 17 00:00:00 2001 From: Neil Martin Date: Thu, 1 Oct 2026 22:15:10 +0100 Subject: [PATCH 2/4] fix(scope): match a resolved device by ID alone, and name the read privilege on a 403 A resolved --computer/--mobile-device carries the device's numeric ID as its name, and add, remove and the verification read also compared it against each member's name and UDID. A different device whose name is the same digits then counted as already in scope on add, was removed alongside it on remove, and stood in for it in the read-back. A resolved target now matches item.ID only; an unresolved literal (remove's fallback) still matches by name, ID or UDID. A 401/403 on the inventory lookup now keeps its exit code and adds a hint naming Read Computers / Read Mobile Devices, since the lookup is the new requirement and a numeric ID that worked before now fails there. Co-Authored-By: Claude Opus 5.5 --- internal/resolve/identifier.go | 53 +++++++++---- internal/resolve/identifier_test.go | 55 ++++++++++++++ internal/scope/commands.go | 4 +- internal/scope/scope.go | 26 +++++-- internal/scope/scope_test.go | 113 ++++++++++++++++++++++++++++ internal/scope/types.go | 22 ++++++ internal/scope/verify.go | 21 ++++-- 7 files changed, 262 insertions(+), 32 deletions(-) diff --git a/internal/resolve/identifier.go b/internal/resolve/identifier.go index 3dd9ee3d..5641f815 100644 --- a/internal/resolve/identifier.go +++ b/internal/resolve/identifier.go @@ -9,6 +9,7 @@ import ( "net/url" "strings" + "github.com/Jamf-Concepts/jamf-cli/internal/exitcode" "github.com/Jamf-Concepts/jamf-cli/internal/registry" ) @@ -38,29 +39,34 @@ type identifierSpec struct { // fields are the RSQL fields holding the name, UDID and serial number. nameField, udidField, serialField string parse func(map[string]any) (*DeviceIdentifiers, error) + // readPrivilege is the Jamf Pro privilege the lookup needs, named in the + // hint on a 401 or 403. + readPrivilege string } // Both endpoints are published on the platform gateway; the per-ID inventory // paths the other resolvers use are not all, so the ID goes through the filter // as well. var computerIdentifierSpec = identifierSpec{ - label: "computer", - path: "/v4/computers-inventory?section=GENERAL§ion=HARDWARE", - idField: "id", - nameField: "general.name", - udidField: "udid", - serialField: "hardware.serialNumber", - parse: parseComputerInventory, + label: "computer", + path: "/v4/computers-inventory?section=GENERAL§ion=HARDWARE", + idField: "id", + nameField: "general.name", + udidField: "udid", + serialField: "hardware.serialNumber", + parse: parseComputerInventory, + readPrivilege: "Read Computers", } var mobileIdentifierSpec = identifierSpec{ - label: "mobile device", - path: "/v2/mobile-devices/detail?section=GENERAL§ion=HARDWARE", - idField: "mobileDeviceId", - nameField: "displayName", - udidField: "udid", - serialField: "serialNumber", - parse: parseMobileDevice, + label: "mobile device", + path: "/v2/mobile-devices/detail?section=GENERAL§ion=HARDWARE", + idField: "mobileDeviceId", + nameField: "displayName", + udidField: "udid", + serialField: "serialNumber", + parse: parseMobileDevice, + readPrivilege: "Read Mobile Devices", } // identifierPageSize bounds the lookup. One exact identifier names a handful of @@ -111,7 +117,7 @@ func resolveIdentifier(ctx context.Context, client registry.HTTPClient, spec ide records, _, err := fetchInventoryPage(ctx, client, path) if err != nil { - return nil, fmt.Errorf("looking up %s %q: %w", spec.label, value, err) + return nil, lookupError(spec, value, err) } var matches []*DeviceIdentifiers @@ -142,6 +148,23 @@ func resolveIdentifier(ctx context.Context, client registry.HTTPClient, spec ide value, len(matches), spec.label, strings.Join(described, ", ")) } +// lookupError wraps a failed inventory lookup. A 401 or 403 gets a hint naming +// the read privilege. Resolving a device is a new inventory read, so an API +// client that could scope a computer by numeric ID before now fails here, and +// the bare status does not say which privilege is missing. +func lookupError(spec identifierSpec, value string, err error) error { + msg := fmt.Sprintf("looking up %s %q", spec.label, value) + var ee *exitcode.Error + if !errors.As(err, &ee) || (ee.Code != exitcode.PermissionDenied && ee.Code != exitcode.Authentication) { + return fmt.Errorf("%s: %w", msg, err) + } + hint := fmt.Sprintf("resolving a %s to its ID reads inventory, so the API client needs %s", spec.label, spec.readPrivilege) + if ee.Hint != "" { + hint += "; " + ee.Hint + } + return &exitcode.Error{Code: ee.Code, Message: msg, Err: err, Hint: hint, Details: ee.Details} +} + // matchedBy names the identifier a record matched value on, or "" for none. func matchedBy(d *DeviceIdentifiers, value string) string { switch { diff --git a/internal/resolve/identifier_test.go b/internal/resolve/identifier_test.go index 71b0cbb9..ce1c4127 100644 --- a/internal/resolve/identifier_test.go +++ b/internal/resolve/identifier_test.go @@ -6,8 +6,13 @@ import ( "context" "errors" "fmt" + "io" + "net/http" "strings" "testing" + + "github.com/Jamf-Concepts/jamf-cli/internal/exitcode" + "github.com/Jamf-Concepts/jamf-cli/internal/registry" ) func inventoryPage(records ...string) string { @@ -125,3 +130,53 @@ func TestResolveMobileDeviceIdentifier_DetailShape(t *testing.T) { } } } + +// statusErrClient answers every request the way client.Do answers a non-2xx: +// with an *exitcode.Error and no response. +type statusErrClient struct{ err error } + +func (c statusErrClient) Do(context.Context, string, string, io.Reader) (*http.Response, error) { + return nil, c.err +} + +// A 401 or 403 on the lookup names the read privilege the resolution needs, +// keeps the exit code, and keeps the hint the client already attached. +func TestResolveDeviceIdentifier_PermissionFailureNamesThePrivilege(t *testing.T) { + cases := []struct { + resolve func(context.Context, registry.HTTPClient, string) (*DeviceIdentifiers, error) + code int + privilege string + }{ + {ResolveComputerIdentifier, exitcode.PermissionDenied, "Read Computers"}, + {ResolveMobileDeviceIdentifier, exitcode.PermissionDenied, "Read Mobile Devices"}, + {ResolveComputerIdentifier, exitcode.Authentication, "Read Computers"}, + } + for _, tc := range cases { + upstream := exitcode.New(tc.code, "permission denied (HTTP 403)").WithHint("upstream hint") + _, err := tc.resolve(context.Background(), statusErrClient{upstream}, "5") + var ee *exitcode.Error + if !errors.As(err, &ee) { + t.Fatalf("want an *exitcode.Error, got %T %v", err, err) + } + if ee.Code != tc.code { + t.Errorf("exit code = %d, want %d", ee.Code, tc.code) + } + if !strings.Contains(ee.Hint, tc.privilege) || !strings.Contains(ee.Hint, "upstream hint") { + t.Errorf("hint %q should name %s and keep the upstream hint", ee.Hint, tc.privilege) + } + if !strings.Contains(err.Error(), `looking up`) || !strings.Contains(err.Error(), "HTTP 403") { + t.Errorf("message should say what was looked up and keep the status: %v", err) + } + if errors.Is(err, ErrNoDeviceMatch) { + t.Error("a permission failure must not read as no match") + } + } + + // Any other failure is wrapped as before, with no privilege hint. + other := exitcode.New(exitcode.General, "server error (HTTP 500)") + _, err := ResolveComputerIdentifier(context.Background(), statusErrClient{other}, "5") + var ee *exitcode.Error + if errors.As(err, &ee) && strings.Contains(ee.Hint, "Read Computers") { + t.Errorf("a 5xx must not be blamed on a missing privilege: %q", ee.Hint) + } +} diff --git a/internal/scope/commands.go b/internal/scope/commands.go index e4f4932d..fe903ca3 100644 --- a/internal/scope/commands.go +++ b/internal/scope/commands.go @@ -120,7 +120,7 @@ func newScopeAddCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { return err } - if !AddToScope(s, section, target.FlagName, target.Name) { + if !AddTargetToScope(s, section, target) { fmt.Fprintf(os.Stderr, "%s %s already in %s scope of %s\n", target.FlagName, target.display(), section, ref) return nil @@ -179,7 +179,7 @@ func newScopeRemoveCmd(ctx *registry.CLIContext, res Resource) *cobra.Command { return err } - if !RemoveFromScope(s, section, target.FlagName, target.Name) { + if !RemoveTargetFromScope(s, section, target) { fmt.Fprintf(os.Stderr, "%s %s not found in %s scope of %s\n", target.FlagName, target.display(), section, ref) return nil diff --git a/internal/scope/scope.go b/internal/scope/scope.go index 1f9a57a6..31e61c36 100644 --- a/internal/scope/scope.go +++ b/internal/scope/scope.go @@ -334,15 +334,21 @@ func silentDropError(singularKey, section, flagName, itemName string, expectedPr // used to carry is gone, the server keeping that element in step with // by itself (see ScopeXML). func AddToScope(s *ScopeXML, section, flagName, name string) bool { + return AddTargetToScope(s, section, ScopeTarget{FlagName: flagName, Name: name}) +} + +// AddTargetToScope is AddToScope for a ScopeTarget, so a device that +// resolveDeviceTarget resolved is matched by its ID alone (see +// ScopeTarget.matches). +func AddTargetToScope(s *ScopeXML, section string, t ScopeTarget) bool { + flagName, name := t.FlagName, t.Name items := getOrCreateScopeItems(s, section, flagName) if items == nil { return false } for _, item := range items.Items { - if strings.EqualFold(item.Name, name) || - (item.ID != "" && item.ID == name) || - (item.UDID != "" && strings.EqualFold(item.UDID, name)) { + if t.matches(item) { return false } } @@ -363,7 +369,13 @@ func AddToScope(s *ScopeXML, section, flagName, name string) bool { // RemoveFromScope removes a named item from the given scope section. Returns // true if removed, false if not found (idempotent no-op). func RemoveFromScope(s *ScopeXML, section, flagName, name string) bool { - return removeNamedItem(readScopeItems(s, section, flagName), name) + return RemoveTargetFromScope(s, section, ScopeTarget{FlagName: flagName, Name: name}) +} + +// RemoveTargetFromScope is RemoveFromScope for a ScopeTarget, matched as +// AddTargetToScope matches. +func RemoveTargetFromScope(s *ScopeXML, section string, t ScopeTarget) bool { + return removeNamedItem(readScopeItems(s, section, t.FlagName), t) } // OutputScope writes the scope to the output formatter. The column formats get @@ -453,16 +465,14 @@ func FlattenScope(s *ScopeXML) []map[string]any { // ─── Internal helpers ───────────────────────────────────────────────────────── -func removeNamedItem(items *ScopeItemSlice, name string) bool { +func removeNamedItem(items *ScopeItemSlice, t ScopeTarget) bool { if items == nil { return false } var keep []NamedItem found := false for _, item := range items.Items { - if strings.EqualFold(item.Name, name) || - (item.ID != "" && item.ID == name) || - (item.UDID != "" && strings.EqualFold(item.UDID, name)) { + if t.matches(item) { found = true continue } diff --git a/internal/scope/scope_test.go b/internal/scope/scope_test.go index aaf5c76d..413c3a6a 100644 --- a/internal/scope/scope_test.go +++ b/internal/scope/scope_test.go @@ -1376,3 +1376,116 @@ func TestResolveDeviceTarget_NonDeviceFlagIsUntouched(t *testing.T) { t.Errorf("got %+v, %v", got, err) } } + +// A resolved device is matched by its ID alone. Another device whose name is +// the same digits must not count as already present on add, be removed with it +// on remove, or stand in for it in the verification read. +func TestResolvedDeviceIsMatchedByIDOnly(t *testing.T) { + resolved := ScopeTarget{ + FlagName: flagComputer, + Name: "107", + Input: "FWWT058JG5", + Device: &resolve.DeviceIdentifiers{ID: "107", Name: "ARMADA-058JG5"}, + } + namesake := NamedItem{ID: "31", Name: "107"} + + s := &ScopeXML{} + s.Computers.Items = []NamedItem{namesake} + if !AddTargetToScope(s, SectionTarget, resolved) { + t.Fatal(`add treated computer 31, named "107", as computer 107 already in scope`) + } + if len(s.Computers.Items) != 2 || s.Computers.Items[1].ID != "107" { + t.Fatalf("want the namesake kept and id 107 appended: %+v", s.Computers.Items) + } + if AddTargetToScope(s, SectionTarget, resolved) { + t.Error("add of a member already present by ID must be a no-op") + } + + if !RemoveTargetFromScope(s, SectionTarget, resolved) { + t.Fatal("remove did not find computer 107") + } + if len(s.Computers.Items) != 1 || s.Computers.Items[0] != namesake { + t.Fatalf(`remove took the computer named "107" as well: %+v`, s.Computers.Items) + } + + if targetPresent(&s.Computers, resolved) { + t.Error(`verification counted the computer named "107" as computer 107`) + } + + // A target that did not resolve (remove's fallback) still matches by name, + // ID or UDID, so a member whose inventory record is gone can be removed. + literal := ScopeTarget{FlagName: flagComputer, Name: "107"} + if !targetPresent(&s.Computers, literal) { + t.Error("an unresolved literal value should still match a member by name") + } +} + +// DiffScope leaves out only the touched member itself, not a different member +// whose name is the touched device's ID. +func TestDiffScope_ResolvedTouchedMemberIsExcludedByID(t *testing.T) { + touched := ScopeTarget{FlagName: flagComputer, Name: "107", Device: &resolve.DeviceIdentifiers{ID: "107"}} + sent := &ScopeXML{} + sent.Computers.Items = []NamedItem{{ID: "31", Name: "107"}, {ID: "107"}} + got := &ScopeXML{} + drops := DiffScope(sent, got, SectionTarget, touched) + if len(drops) != 1 || len(drops[0].Missing) != 1 || drops[0].Missing[0] != "107" { + t.Fatalf(`want the dropped namesake (label "107") reported and the touched member left out: %+v`, drops) + } +} + +// The command, not just the helpers, must match a resolved device by ID: a +// remove of computer 107 on a scope holding only computer 31, which is named +// "107", finds nothing and sends no write. +func TestScopeRemoveCommand_DoesNotTakeANamesakeOfTheResolvedID(t *testing.T) { + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + return &resolve.DeviceIdentifiers{ID: "107", Name: "ARMADA-058JG5"}, nil + }) + client := &mockPutClient{getBody: `5P` + + `31107`} + res := Resource{APIPath: "policies", SingularKey: "policy", CLIName: "classic-policies"} + cmd := newScopeRemoveCmd(®istry.CLIContext{Client: client}, res) + cmd.SetArgs([]string{"5", "--computer", "FWWT058JG5"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + for _, r := range client.requests { + if !strings.HasPrefix(r, "GET ") { + t.Fatalf(`remove of computer 107 wrote the scope, taking computer 31 named "107": %v`, client.requests) + } + } +} + +// And add of computer 107 is not a no-op because computer 31 is named "107". +func TestScopeAddCommand_ANamesakeOfTheResolvedIDIsNotAlreadyPresent(t *testing.T) { + stubResolveDevice(t, func(_, _ string) (*resolve.DeviceIdentifiers, error) { + return &resolve.DeviceIdentifiers{ID: "107", Name: "ARMADA-058JG5"}, nil + }) + client := &mockPutClient{getBody: `5P` + + `31107`} + res := Resource{APIPath: "policies", SingularKey: "policy", CLIName: "classic-policies"} + cmd := newScopeAddCmd(®istry.CLIContext{Client: client, Output: &captureFormatter{}}, res) + cmd.SetArgs([]string{"5", "--computer", "FWWT058JG5"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + // The canned GET never shows computer 107, so the verification read must + // report it missing rather than accept computer 31 named "107" in its place. + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "is not in the target scope") { + t.Errorf(`verification took computer 31 named "107" for computer 107: %v`, err) + } + + var put string + for i, r := range client.requests { + if strings.HasPrefix(r, "PUT ") { + put = client.bodies[i] + } + } + if put == "" { + t.Fatalf(`add of computer 107 was skipped as already present because computer 31 is named "107": %v`, client.requests) + } + if !strings.Contains(put, "107") || !strings.Contains(put, "31") { + t.Errorf("want computer 107 added beside computer 31:\n%s", put) + } +} diff --git a/internal/scope/types.go b/internal/scope/types.go index 3fe56adf..9ea4e575 100644 --- a/internal/scope/types.go +++ b/internal/scope/types.go @@ -56,6 +56,28 @@ func (t ScopeTarget) display() string { return fmt.Sprintf("%q (id %s, %s)", t.Input, t.Device.ID, t.Device.Name) } +// matches reports whether item is the member t names. A resolved device +// (Device set) is matched by its ID alone: Name then holds that ID, and also +// comparing it against each member's name and UDID would let a different device +// whose name is the same digits count as "already in scope" on add, and be +// removed alongside it on remove. Anything else matches by name, ID or UDID, +// since the server augments what it was sent (a member sent by name comes back +// with an ID). +func (t ScopeTarget) matches(item NamedItem) bool { + if t.Device != nil { + return item.ID == t.Device.ID + } + return namedItemMatches(item, t.Name) +} + +// namedItemMatches matches a member by name or UDID, case-insensitively, or by +// ID exactly. +func namedItemMatches(item NamedItem, value string) bool { + return strings.EqualFold(item.Name, value) || + (item.ID != "" && item.ID == value) || + (item.UDID != "" && strings.EqualFold(item.UDID, value)) +} + // ─── XML types ───────────────────────────────────────────────────────────────── // These model the Classic API scope XML structure. Custom XML marshalers on the // slice types handle the parent/child nesting (e.g. wrapping diff --git a/internal/scope/verify.go b/internal/scope/verify.go index fd639465..b6da3fe8 100644 --- a/internal/scope/verify.go +++ b/internal/scope/verify.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "sort" - "strings" "github.com/Jamf-Concepts/jamf-cli/internal/registry" ) @@ -42,7 +41,7 @@ func VerifyScopeWrite(ctx context.Context, client registry.HTTPClient, res Resou // The member the command changed, reported in its own words: absent when // it should be there is usually an identifier that named no record, which // is a different diagnosis from a category the server refused to keep. - if items := readScopeItems(got, touchedSection, touched.FlagName); itemPresent(items, touched.Name) != expectPresent { + if items := readScopeItems(got, touchedSection, touched.FlagName); targetPresent(items, touched) != expectPresent { return silentDropError(res.SingularKey, touchedSection, touched.FlagName, touched.display(), expectPresent) } @@ -77,7 +76,7 @@ func DiffScope(sent, got *ScopeXML, touchedSection string, touched ScopeTarget) var missing []string for _, item := range before.Items { label := itemLabel(item) - if section == touchedSection && flag == touched.FlagName && strings.EqualFold(label, touched.Name) { + if section == touchedSection && flag == touched.FlagName && touched.matches(item) { continue } if !itemPresent(after, label) { @@ -109,13 +108,21 @@ func itemLabel(item NamedItem) string { // it was sent (a member sent by name comes back with an ID, and a network // segment gains a uid), so equality of the elements is not the test. func itemPresent(items *ScopeItemSlice, value string) bool { - if items == nil || value == "" { + if value == "" { + return false + } + return targetPresent(items, ScopeTarget{Name: value}) +} + +// targetPresent is itemPresent for the member a command touched, matched as +// ScopeTarget.matches matches it: a resolved device by its ID alone, so a +// different device whose name is the same digits cannot stand in for it. +func targetPresent(items *ScopeItemSlice, t ScopeTarget) bool { + if items == nil || (t.Device == nil && t.Name == "") { return false } for _, item := range items.Items { - if strings.EqualFold(item.Name, value) || - item.ID == value || - strings.EqualFold(item.UDID, value) { + if t.matches(item) { return true } } From 24fe0f99fb9ede54ee0ef6614b0a098d366b9055 Mon Sep 17 00:00:00 2001 From: Neil Martin Date: Thu, 1 Oct 2026 22:17:21 +0100 Subject: [PATCH 3/4] docs(changelog): record the scope --computer/--mobile-device breaking change Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93fd42ab..d0c10fc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,32 @@ commit types the repo already uses (`feat!`/`build!` for a breaking change). ## Unreleased +### Breaking — `scope add`/`remove` resolve `--computer` and `--mobile-device` to an ID first + +`scope add --computer ` answered `409 Unable to match computer` on every +computer-scoped resource: a Mac's UUID-shaped UDID was sent as ``, and a +matched UDID went out beside an empty ``, which the Classic computer +matcher reads first and refuses. The `scope add` and `scope remove` commands +of the eight scopeable Classic resources now look a `--computer` or +`--mobile-device` value up in inventory — as an ID, name, UDID or serial +number — and send the device's `` alone. That is the one form both Classic +matchers resolve unconditionally. + +Two things a script can see change: + +- **A name shared by more than one device is refused**, naming the matching + ids. The Classic API's own name match picked one of them silently. +- **The API client needs Read Computers or Read Mobile Devices**, because the + lookup reads inventory. Without it, a numeric ID that worked before now + fails with a 403 (exit 5), and the hint names the privilege. + +`scope remove` by serial number now works; before, it could never match, +because the scope GET carries no serial. A value that no longer names any +device still removes a member listed under that ID or name. + +**Migration.** Pass the numeric ID for a device whose name is not unique, and +grant the API client Read Computers / Read Mobile Devices. + ### Breaking — MCP `run_command` refuses local paths and credential output unless the operator allows them `run_command` used to compare the model's raw argument list against a short From b35835876ecfc587da3f8dea95fd5c2c767296b3 Mon Sep 17 00:00:00 2001 From: Neil Martin Date: Fri, 2 Oct 2026 21:25:16 +0100 Subject: [PATCH 4/4] chore: empty commit to re-trigger CI Co-Authored-By: Claude Opus 5.5