[CXH-2156, CXH-2157] Enhance user account provisioning with custom privileges support - #28
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| if isCustomPrivilege && slices.Contains(userAccount.Privileges.JSSObjects, resource.Id.Resource) { | ||
| if isCustomPrivilege && userAccount.Privileges.Contains(resource.Id.Resource) { |
There was a problem hiding this comment.
🟡 Suggestion: Grant matching now spans all 7 privilege categories via Privileges.Contains() instead of only JSSObjects. This is a correctness improvement, but it's an un-gated change to sync output — on the next sync, accounts/groups holding non-JSSObjects privileges (e.g. Recon, Casper*) will gain grants they didn't previously get. Worth calling out in the PR description since it changes emitted grants for existing installs. (medium confidence)
There was a problem hiding this comment.
Documented in the PR description under "Behavior change — existing installs" — no code change needed per the review's own assessment. Confirmed this is intentional: the JSSObjects-only check was a pre-existing gap that only became reachable once this PR added write support for the other 6 categories.
Connector PR Review: [CXH-2156, CXH-2157] Enhance user account provisioning with custom privileges supportBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This PR adds Custom privilege support to Jamf admin-account provisioning: a Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
This PR added 7 custom-privilege profile fields (privileges_jss_objects, _jss_settings, _jss_actions, _recon, _casper_admin, _casper_remote, _casper_imaging) for Custom privilege_set accounts, wired end-to-end in code, but never documented in README/connector.mdx. Also adds a regression-pin test for the shared "member" entitlement slug (group/role/userGroup/site) - none existed, and a future rename would silently orphan every existing grant of these types with no error anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Connector PR Review: [CXH-2156, CXH-2157] Enhance user account provisioning with custom privileges supportBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness, and scoped suggestions to the incremental changes since Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
| } | ||
|
|
||
| if isCustomPrivilege && slices.Contains(group.Privileges.JSSObjects, resource.Id.Resource) { | ||
| if isCustomPrivilege && group.Privileges.Contains(resource.Id.Resource) { |
There was a problem hiding this comment.
Major: This PR's core diff here is a one-line change (slices.Contains(group.Privileges.JSSObjects, ...) → group.Privileges.Contains(...), same at line 126 for userAccount) but it changes the actual grants Grants() produces for every existing tenant: any account/group holding Recon, Casper Admin/Remote/Imaging, or JSS Settings/Actions privileges will now show grants for those role resources on the next sync, where previously only JSSObjects privileges were matched. This is a correct bug fix (the PR description says so plainly), but it's a behavior change bundled into a feature PR, and it will look like a grant spike/diff to anyone watching sync results on an existing install. Recommend calling this out in the release notes / changelog distinctly from the Custom-privileges feature, so support isn't surprised by a 'why did this account suddenly get 5 new grants' question.
Rationale: Per review standards, breaking/behavior changes are always call-outs regardless of how well-justified they are, since downstream consumers (support, customers auditing grants) need to know this is expected and why.
There was a problem hiding this comment.
Agreed this needs its own call-out, distinct from the feature description — that's already in place: the PR description has a dedicated ## Behavior change — existing installs section (separate H2 from ## Summary) spelling out exactly this — which categories newly match, why it's a correctness fix not a regression, and that no resource IDs/entitlement slugs change. This repo doesn't maintain a separate CHANGELOG file; the PR description is what feeds release notes for baton connectors. Let me know if you want it phrased differently or surfaced somewhere else.
|
|
||
| s.mu.Lock() | ||
| // NOTE: same caveat as the account create path below — 409 here is | ||
| // unverified against a live Jamf tenant. See CXH-1487. |
There was a problem hiding this comment.
Nit: These three new NOTE comments (also at ~534 and ~782) cite 'CXH-1487' as the source of the 'unverified against a live tenant' caveat, but CXH-1487 is the parent epic (the original provisioning PR #26) — the actual ticket that raised this specific unverified-409/JSON-error-body concern is CXH-2156 (the audit ticket this PR is partially resolving). Pointing at CXH-1487 sends a future reader to the wrong issue when they go looking for the open question this comment references.
Rationale: Accurate ticket cross-references matter for traceability — CXH-1487 doesn't contain the 409/error-body analysis, CXH-2156 does, and CXH-2156 is still open with these exact items as 'Open questions'.
There was a problem hiding this comment.
Fixed — corrected all three NOTE comments to reference CXH-2156 (the ticket with the actual 409/error-body analysis and open questions) instead of the parent epic CXH-1487. Pushed in 67ba83b.
The unverified-409/JSON-error-body caveat traces to CXH-2156 (the audit ticket with the open questions), not CXH-1487 (the parent epic). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| } | ||
|
|
||
| var privileges *jamf.Privileges | ||
| if privilegeSet == privilegeSetCustom { |
There was a problem hiding this comment.
Asymmetry on the privileges guard: Custom with an empty privileges block fails hard (good), but if the operator fills any privileges_* fields and leaves Privilege Set empty/default (Auditor) — or picks any non-Custom set — those privileges are dropped with no error and no log. Privilege Set is optional with a placeholder, so that path is easy to hit and you get an account with none of the requested privileges.
Reject the same way when privileges are present and privilege_set != Custom, or require Privilege Set when any privileges_* field is set.
| CasperRemote: stringSliceFromProfile(profileMap, profileFieldPrivilegesCasperRemote), | ||
| CasperImaging: stringSliceFromProfile(profileMap, profileFieldPrivilegesCasperImaging), | ||
| } | ||
| if privileges.IsEmpty() { |
There was a problem hiding this comment.
privileges are silently discarded when privilege_set is not Custom since the validation is one-directional.
an operator who fills the seven Privileges fields and leaves Privilege Set at its Auditor default gets an Auditor account with none of the privileges they asked for, and no error explaining why
There was a problem hiding this comment.
Fixed in 92831da: extracted the privilege-resolution logic into resolvePrivileges(), which now rejects the request with an explicit error if any privileges_* field is set while privilege_set isn't Custom — symmetric with the existing empty-Custom check. Added regression tests in userAccount_test.go covering both directions.
| } | ||
|
|
||
| if isCustomPrivilege && slices.Contains(group.Privileges.JSSObjects, resource.Id.Resource) { | ||
| if isCustomPrivilege && group.Privileges.Contains(resource.Id.Resource) { |
There was a problem hiding this comment.
adding privilege grants for ALL roles, including x.PrivilegeSet != "Custom" is expected? do we want to increase the role grants this way? we may want to confirm performance with customers after this chang
Widening slices.Contains(x.Privileges.JSSObjects, ...) to x.Privileges.Contains(...) widens an existing hole: neither branch verifies x.PrivilegeSet == "Custom" before matching against Privileges. Jamf's findaccountsbyid / findgroupsbyid populate the <privileges> block for built-in privilege sets too (an Administrator account carries the full effective privilege list), so every Administrator account/group now matches nearly every privilege role emitted by List — hundreds of individual privilege grants per admin, on top of the Administrator privilege-set grant it already gets. Previously only JSS Objects privileges leaked this way, which masked the scale of it.
Same change needed in the userAccount loop below (line 126). Suggested shape for both:
isCustom := group.PrivilegeSet == privilegeSetCustom
if isCustomPrivilege && isCustom && group.Privileges.Contains(resource.Id.Resource) {
rv = append(rv, grant.NewGrant(resource, memberEntitlement, gr.Id))
continue
}If the intent really is to grant individual privilege roles to built-in-set holders as well, that should be stated in a comment.
There was a problem hiding this comment.
Great catch — fixed in 92831da. Both loops now gate on the account's/group's own PrivilegeSet == privilegeSetCustom before matching against Privileges, extracted into a small matchesIndividualPrivilege() helper with unit tests covering exactly this: an Administrator/Auditor/Enrollment-Only account with populated Privileges no longer matches an individual-privilege role. I ran this through Phase-3 investigation (3 hypotheses) before applying — confirmed the code-level gap and the fix's safety via a deterministic trace, but Jamf's own API docs are silent on whether built-in-set accounts actually get a populated <privileges> block in practice, so I couldn't verify the real-world trigger against a live tenant. Applying the fix regardless since it's free (no-op against current mock/tests) and matches the documented Custom-only intent already stated on jamf.Privileges.
| var knownPrivilegeSets = []string{"Administrator", "Auditor", "Enrollment Only", "Custom"} | ||
| // Valid values Jamf accepts for an admin account's privilege_set. See | ||
| // https://developer.jamf.com/jamf-pro/reference/createaccountbyid. | ||
| const ( |
There was a problem hiding this comment.
[Connector] role.go still uses privilege-set string literals that now have named constants in this package
Good change to name these. role.go:24 declares a second, overlapping list of the same values as literals:
var privilegeSets = []string{
"Administrator",
"Auditor",
"Enrollment Only",
}Two near-identical lists of the same vendor enum in one package will drift — and privilegeSets gates isCustomPrivilege in Grants, so a drift there silently changes which grants are emitted. Reuse the constants:
// privilegeSets are the built-in sets; privilegeSetCustom is deliberately
// excluded — a Custom account's access is described by its individual privileges.
var privilegeSets = []string{privilegeSetAdministrator, privilegeSetAuditor, privilegeSetEnrollmentOnly}There was a problem hiding this comment.
Fixed in 92831da: role.go now reuses privilegeSetAdministrator/privilegeSetAuditor/privilegeSetEnrollmentOnly from userAccount.go instead of a second literal list, with the same comment you suggested explaining why Custom is excluded.
…-only grant gating - CreateAccount now rejects privileges_* fields set on a non-Custom account instead of silently discarding them (previously only the reverse case, Custom with no privileges, was validated). - role.go's Grants() now requires PrivilegeSet == Custom before matching an account/group's Privileges against an individual privilege role, closing a gap that widening JSSObjects-only matching to all 7 categories would otherwise have made more consequential for built-in privilege sets. - role.go's privilegeSets reuses the named consts from userAccount.go instead of a second literal list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
10 participants
Summary
Customprivilege_set support to Jamf admin-account provisioning: ajamf.Privilegestype covering all 7 Classic API categories (JSS Objects, JSS Settings, JSS Actions, Recon, Casper Admin, Casper Remote, Casper Imaging), exposed as newCreateAccountschema fields.Customaccount specifies at least one privilege category, rejecting the create instead of silently provisioning an unusable zero-privilege admin account.MarshalXMLonPrivilegesso only populated categories are sent to Jamf (Go'sencoding/xmlomitemptydoes not elide empty nested-path wrapper elements on its own).Behavior change — existing installs
pkg/connector/role.go'sGrants()now matches privilege "role" resources against all 7Privilegescategories viaPrivileges.Contains(), instead of onlyJSSObjects. This is a correctness fix (the other 6 categories were previously invisible to grant sync even where a Custom account or group actually held those privileges), but it means: on the next sync, any account or group holding non-JSSObjects privileges (Recon, Casper Admin/Remote/Imaging, JSS Settings/Actions) will gain grants they did not previously show. No resource IDs, entitlement slugs, or grant semantics changed — this only affects which existing accounts/groups get matched to already-existing privilege entitlements.Test plan
go build ./...,go vet ./...,go test ./...make lint— 0 issuespkg/jamf/models_test.gocovering XML marshaling (omits empty categories, omits the whole element for non-Custom) and the newPrivileges.Contains/IsEmptyhelpers