Repository navigation
[CXP-1240][CXP-1286] Reduce and improve the cost and speed of synchronization, and fix users shown as "user" - #35
mateovespConductor wants to merge 4 commits into
Conversation
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| userNameUrlPath = "/JSSResource/users/name/%s" // https://developer.jamf.com/jamf-pro/reference/findusersbyname | ||
| oauthTokenUrlPath = "/api/v1/oauth/token" //nolint:gosec // not a token, https://developer.jamf.com/jamf-pro/docs/client-credentials | ||
| privilegesUrlPath = "/api/v1/api-role-privileges" // https://developer.jamf.com/jamf-pro/reference/get_v1-api-role-privileges | ||
| proUsersUrlPath = "/api/v1/users" // https://developer.jamf.com/jamf-pro/reference/get_v1-users |
There was a problem hiding this comment.
🟡 Suggestion (confidence: high that it's real, medium on severity): User List, and the user/admin grants in site and role, now go through /api/v1/users, /api/v1/accounts and /api/v1/sites/{id}/objects with no fallback. Per the PR body, /api/v1/users needs Jamf Pro ≥ 11.26, so an older on-prem instance fails every sync with FailedPrecondition after this deploys. The PR body says the README documents this, but neither README.md nor docs/connector.mdx mentions 11.26. Please document the minimum version, and consider falling back to the Classic list or gating the change.
There was a problem hiding this comment.
+1. I grepped README, connector.mdx and docs-info.md for 11.26 and found nothing, even though the PR body says it's in the README. docs-info.md:70 still says no endpoint is gated. The minimums for /api/v1/accounts and /sites/{id}/objects aren't documented anywhere, and user, userAccount, site and role sync all depend on them now.
There was a problem hiding this comment.
Thanks, fixed in fe05279: README and connector.mdx state the minimum, 11.26.0, and docs-info lists each endpoint's version (users 11.26.0, accounts 11.20.0, site objects 11.2.0). No Classic fallback, since it would bring back the per-item reads this PR removes.
| time.Sleep(time.Second) | ||
| // groupGrants lists the grants of one group: its members, then the roles and the site it holds. | ||
| func (g *groupResourceType) groupGrants(ctx context.Context, store sessions.SessionStore, resource *v2.Resource, groupId int) ([]*v2.Grant, error) { | ||
| group, err := g.client.GetGroupDetails(ctx, groupId) |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): The removed loop worked around a documented Jamf bug: /JSSResource/accounts/groupid/{id} sometimes returns an empty members list for a group that has members. Without it, one empty response now makes Grants() return zero member grants, which C1 treats as revoking every member of that group (section S data-loss shape, though no error is swallowed here). If the bug is confirmed fixed, cite the evidence in a comment. Otherwise consider cross-checking an empty list, e.g. by retrying, or by checking the Pro /api/v1/accounts list for accounts in the group, before emitting an empty membership.
There was a problem hiding this comment.
+1. "Members come from the group detail" is the same /JSSResource/accounts/groupid/{id} endpoint the original HACK (922cc5b, BB-398) was about, so the bug is still there. To be fair, the old loop never re-read fresh (no WithFreshReads), so it wasn't really protecting anything either. But the ticket asked to keep the retry with fresh reads and respect ctx, and Grant/Revoke plus the README still treat an empty read as untrustworthy. 1-2 fresh re-reads on empty would close it.
There was a problem hiding this comment.
Thanks for digging into the history! We're keeping it out: an empty read for a populated group doesn't reproduce on a real tenant, and we don't keep retry loops in sync for vendor flakiness. Grant/Revoke still refuse to write over an empty list, which is where a bad read could actually drop members.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
- List users, admin accounts and site members through the Jamf Pro API, SDK-paged (1000 / 100 / 1000 per page) - Role grants type-scoped: privilege-set phase, then Custom accounts (10 per page, one detail read each) - Group grants from one group read: members, held roles and site; empty-member retry loop removed - Site grants in bag phases: users, user groups, admin accounts - StaticEntitlements + SkipEntitlements on group, userGroup, site and role; role descriptions on the resource - Grant pages capped at 1000 with a keyset cursor - Session store: privilege catalog read once; username/email -> user id written by user List, read by device Grants - Remove getUserIndex, the in-process caches and the per-item Classic detail reads - Device Grant resolves the replaced grant through a Pro users RSQL lookup - Pro API 404 -> FailedPrecondition (users, accounts, site objects, privileges) - userAccount display name uses FullName - CreateAccount/Delete input errors -> InvalidArgument; Delete refuses the connector's own account - Enable the session store; move resource types to resource_types.go - Mock: Pro users, accounts and site objects endpoints; README sync-cost section; openapi spec; capabilities regenerated - Tests for grant paging, role/site/group grant phases, device owner lookup, Pro client paging and 404 mapping
491af91 to
2e632ac
Compare
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Big improvement overall. IDs, slugs and grant IDs all match v0.2.4, and the keyset paging, TypeScopedGrants and session store wiring look right. A few things inline, and +1 on two of the bot's threads.
One thing from the ticket that isn't touched: the customer's "users named user". The user display name is still just user.FullName (user.go:60) with no fallback, so anyone without a real name gets an empty display name. I can't prove that's what they're seeing, but it's the obvious suspect. Maybe fall back to FullName, then Username, then Email?
Also FYI, c1 still runs Jamf in-process at v0.0.9 with no session store wired, so none of this reaches hosted customers yet, and a straight bump would hit the session-store paths.
| syncRoles: opts.WillSyncResourceType(resourceTypeRole.Id), | ||
| syncSites: opts.WillSyncResourceType(resourceTypeSite.Id), | ||
| syncUserAccounts: opts.WillSyncResourceType(resourceTypeUserAccount.Id), | ||
| syncManagedDevices: opts.WillSyncResourceType(resourceTypeManagedDevice.Id), |
There was a problem hiding this comment.
small one: WillSyncResourceType returns true when there's no type filter, so user List fills the username/email cache (about 40 session RPCs per 1000 users) even when devices aren't being synced. Not a bug, just extra work.
There was a problem hiding this comment.
With no filter every type syncs, devices included, so the cache is needed. With selective sync, c1 sends an explicit list and the cache is skipped when devices are off.
…ssion store (1000 member ids per key) - User display name falls back to the username, then the email (CXP-1286) - README, connector.mdx and docs-info state the Jamf Pro minimum versions - README sync cost: one read per user group - Tests: user group paging with one Jamf read and a store miss; group tests reuse groupPrincipal
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
mateovespConductor
changed the title
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Thanks, the user group paging, the display-name fallback and the version docs all look good. I checked the versions against the Jamf changelogs and they match. One small thing inline.
| if err != nil { | ||
| return userGroupMemberPage{}, err | ||
| } | ||
| pages := memberPages(group.Users) |
There was a problem hiding this comment.
edge case: after a store miss on page k, this re-reads the group and returns pages[k] from the new list, but pages 0..k-1 came from the old read. If someone with a lower id left in between, everything shifts one slot and the member at the page boundary gets skipped. If someone joined, one member comes out twice. It's rare (needs a miss plus a membership change mid-sync), but carrying the last emitted user id in the token and filtering id > cursor on a re-read would close it, the way pageGrants did.
…-read after a store miss keeps only the ids above it - Site grants only read the users, user groups and admin accounts the sync includes - Every client method returns Jamf's rate-limit annotations; List and Grants pass them to the SDK, and Grant/Revoke/CreateAccount/Delete return the write's - Docs: empty-member guard reworded; Delete refuses the connector's own account; site sync cost per synced type - Tests: user group store miss after a member leaves; site sync without userAccount, or without any of its principal types, reads nothing it doesn't need
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
| // if this endpoint becomes reliable again, we can remove this for loop | ||
| var group *jamf.Group | ||
| count := 0 | ||
| for count < 5 { |
There was a problem hiding this comment.
Why was this removed? If the previous code comment is right, then we probably should keep something that help us avoid this "returns empty without failing" situation.
If the comment wasn't right, then I agree on removing it
There was a problem hiding this comment.
The comment was true when it was written: #14 has a video of it happening in May 2025. Today I couldn't reproduce it on the trial: about 4,200 reads with both auth types, in parallel and right after membership changes, and the members came back every time. The loop also wasn't protecting us. Its retries got the cached response back, and after 5 empty reads it carried on with no members anyway. Grant/Revoke still refuse to write over an empty read, which is where a false empty would do real damage
|
|
||
| nextCursor := page[len(page)-1].GetId() | ||
| if nextCursor == "" { | ||
| return nil, "", errors.New("jamf-connector: grants page has more items but an empty cursor") |
There was a problem hiding this comment.
nit: new errors in this PR use the legacy jamf-connector: prefix (here and in group, role, site, userGroup, userAccount). CLAUDE.md asks for baton-jamf:; consider switching the new ones.
There was a problem hiding this comment.
Thanks! I'll switch all of them in a follow-up PR, together with renaming the files from camelCase to snake_case, so this one stays focused and the prefix doesn't end up mixed.
| } | ||
|
|
||
| return rv, nil, nil | ||
| page, nextCursor, err := pageGrants(grants, attrs.PageToken.Token) |
There was a problem hiding this comment.
nit: each grants page calls groupGrants again, so every page re-reads GetGroupDetails (and the privilege catalog for Custom groups). Consider storing later pages in the session store, as userGroup does with cacheUserGroupMemberPages/memberPage.
| } | ||
| var held []string | ||
| for _, privilege := range dedupeStrings(privileges.All()) { | ||
| if slices.Contains(catalog, privilege) { |
There was a problem hiding this comment.
nit: slices.Contains over the full catalog makes this O(n·m) per Custom account/group. Consider building a map[string]struct{} from the catalog once (e.g. in privilegeCatalog) and checking against that.
| "user_id": fmt.Sprintf("account:%d", account.ID), | ||
| } | ||
|
|
||
| displayName := account.FullName |
There was a problem hiding this comment.
nit: the display name now uses FullName, but first_name/last_name still come from SplitFullName(account.Name) (the username). Use the same FullName → Name fallback for both.
There was a problem hiding this comment.
I kept first_name/last_name as they shipped, since changing a published profile value can break customer CEL and attribute mappings that read them. The real name is now in a new full_name key, the same one user profiles have, and the display name already uses it
…-read after a store miss keeps only the ids above it - Group grants read the group once; a group with more than one page is stored for its later pages - Site grants only read the users, user groups and admin accounts the sync includes - Every client method returns Jamf's rate-limit annotations; List and Grants pass them to the SDK, and Grant/Revoke/CreateAccount/Delete return the write's - Custom privileges are checked against a set built from the catalog - Admin account profiles gain full_name; first and last name keep their shipped values - Docs: empty-member guard reworded; Delete refuses the connector's own account; group and site sync cost - Tests: user group and group store misses; site sync without userAccount, or without any of its principal types, reads nothing it doesn't need
Superseded — see the current review report for commit
|
Connector PR Review: [CXP-1240][CXP-1286] Reduce and improve the cost and speed of synchronization, and fix users shown as "user"Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commits change how group Grants page:
Coverage and criteria:
Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
| } | ||
|
|
||
| // The group read supplies the annotations. | ||
| catalog, err := privilegeCatalogSet(ctx, store, g.client) |
There was a problem hiding this comment.
[Connector] Privilege catalog read drops its rate-limit annotations
For a Custom group, heldRoles reads the privilege catalog after the group, but privilegeCatalogSet discards that call's annotations (session_cache.go:53). When the catalog isn't in the store yet, the page reports the group read's rate-limit state rather than the most recent call's.
| } | ||
| response, err := c.wrapper.Do(request) | ||
| var rateLimit v2.RateLimitDescription | ||
| response, err := c.wrapper.Do(request, uhttp.WithRatelimitData(&rateLimit)) |
There was a problem hiding this comment.
[Client] Jamf error bodies aren't parsed
Do is called without uhttp.WithErrorResponse, so when the new Pro API endpoints (/api/v1/users, /api/v1/accounts, /api/v1/sites/{id}/objects, /api/v1/api-role-privileges) fail, Jamf's error message doesn't reach the returned error. Only 401 and 404 get a hand-written message (here and in errors.go).
| - Jamf has two distinct account types: **Users** (directory users) and **User Accounts** (Jamf Pro console admins). The connector can only create **one** of these types per connector instance — set by the **Account Provisioning Target** configuration field. Deletion works for both types regardless of this setting. Created accounts are `Enabled`, `Full Access`, and default to the `Auditor` privilege set; a random password is generated and returned once, as plaintext. See [Jamf account privileges](#jamf-account-privileges) below for the privileges this requires. | ||
| - Jamf has two distinct account types: **Users** (directory users) and **User Accounts** (Jamf Pro console admins). The connector can only create **one** of these types per connector instance — set by the **Account Provisioning Target** configuration field. Deletion works for both types regardless of this setting, except for the user account the connector authenticates with, which Delete refuses. Created accounts are `Enabled`, `Full Access`, and default to the `Auditor` privilege set; a random password is generated and returned once, as plaintext. See [Jamf account privileges](#jamf-account-privileges) below for the privileges this requires. | ||
| - **User Groups**: Grant/Revoke adds or removes a Jamf user from a **static** User Group only — Smart User Groups are rejected. | ||
| - **Sites**: Grant/Revoke is only supported for the `user` principal — a Jamf user's site membership is a true multi-valued list. `userGroup`/`userAccount`/`group` site membership remains sync-only (each of those objects can only belong to one site at a time, so granting a new one would silently displace the old one instead of adding a membership). |
There was a problem hiding this comment.
[Docs] Notes explain internal mechanics
A few parentheticals in these notes explain how Jamf works rather than what the admin can do:
- here: "(each of those objects can only belong to one site at a time, so granting a new one would silently displace the old one…)"
- L33: "(an ambiguous empty read is never written over, or assumed to mean someone was removed)"
- L34: "(Jamf's lowest access, and not itself revocable)" and "(that membership granted it nothing)"
| </Note> | ||
|
|
||
| Tested with Jamf Pro 11.32.1. Managed Devices use the Jamf Pro API v4 computers-inventory and v2 mobile-devices endpoints. | ||
| Requires Jamf Pro 11.26.0 or later (Managed Devices: 11.30.0). Tested with Jamf Pro 11.32.1. |
There was a problem hiding this comment.
[Docs] Test evidence in the docs
"Tested with Jamf Pro 11.32.1" is test evidence, not an instruction, and it goes stale with the next Jamf release. Same sentence in README.md:18.
| `Read Accounts`/`Create Accounts`/`Update Accounts`/`Delete Accounts` cover both admin user accounts and admin groups — Jamf has no separate privilege for account groups. Every write path reads the object first, so each `Update` privilege above needs its `Read` counterpart too. | ||
|
|
||
| Jamf reports a missing privilege differently depending on which API it uses: the Classic API (`/JSSResource` — Users, Accounts, Static User Groups) answers with HTTP 401, and the Jamf Pro API (`/api/v1`, `/api/v2`, `/api/v4` — the roles catalog, Computers, Mobile Devices) with HTTP 403. The connector reports both as a permission error. | ||
| Jamf reports a missing privilege differently depending on which API it uses: the Classic API (`/JSSResource` — User Groups, admin groups, Sites, and user and account details and writes) answers with HTTP 401, and the Jamf Pro API (`/api/v1`, `/api/v2`, `/api/v4` — the users and accounts lists, site objects, the roles catalog, Computers, Mobile Devices) with HTTP 403. The connector reports both as a permission error. |
There was a problem hiding this comment.
[Docs] 401 vs 403 paragraph is implementation detail
Which API returns 401 and which returns 403, with the /JSSResource and /api/v1 paths, is internal behavior. An admin only needs to know that a missing privilege shows up as a permission error.
| list even when the group genuinely has members, and the connector refuses to | ||
| write over (or assume someone was removed from) a list it can't trust is | ||
| complete. Only Group Access | ||
| instead of reporting success. The write replaces the whole member list, so the |
There was a problem hiding this comment.
[Docs] Reasoning instead of behavior
"The write replaces the whole member list, so the connector never writes over an empty read… if that read were incomplete, the write would drop every real member" explains why the guard exists. The behavior (first member can't be added, Revoke returns a retryable error) is already stated in the lines above.
| - Jamf Pro API: `POST /api/v1/auth/token` or `POST /api/v1/oauth/token` (authentication), `GET /api/v1/auth` (validation), `GET /api/v1/api-role-privileges` (Role sync), `GET /api/v4/computers-inventory` and `PATCH /api/v4/computers-inventory-detail/{id}` (Managed Device computers), `GET`/`PATCH /api/v2/mobile-devices[/{id}]` (Managed Device mobile devices). | ||
| - Jamf Pro API: `POST /api/v1/auth/token` or `POST /api/v1/oauth/token` (authentication), `GET /api/v1/auth` (validation and the delete self-check), `GET /api/v1/api-role-privileges` (Role sync), `GET /api/v1/users` (User sync and managed device owner lookup), `GET /api/v1/accounts` (User Account sync, Role and Site grants), `GET /api/v1/sites/{id}/objects` (Site grants), `GET /api/v4/computers-inventory` and `PATCH /api/v4/computers-inventory-detail/{id}` (Managed Device computers), `GET`/`PATCH /api/v2/mobile-devices[/{id}]` (Managed Device mobile devices). | ||
|
|
||
| Version requirements: Jamf Pro 11.26.0 or later. `GET /api/v1/users` was added in 11.26.0, `GET /api/v1/accounts` in 11.20.0, `GET /api/v1/sites/{id}/objects` in 11.2.0 and `GET /api/v1/api-role-privileges` in 10.49.0; an older instance fails the sync with `FailedPrecondition` naming the missing endpoint. Managed Devices (opt-in) need 11.30.0 for `/api/v4/computers-inventory`. |
There was a problem hiding this comment.
[Docs] Per-endpoint minimum versions aren't verifiable from the code
/api/v1/accounts in 11.20.0, /api/v1/sites/{id}/objects in 11.2.0 and /api/v1/api-role-privileges in 10.49.0 can't be checked against anything in the repo. What's the source for these (Jamf release notes, API changelog)?
There was a problem hiding this comment.
[Docs] "ConductorOne" instead of "C1"
The docs prose uses "C1", but this file still says "ConductorOne" in the frontmatter description and og:description (L4-5), the Account Provisioning Target field (L163, "Which Jamf account type ConductorOne should create") and the download link text (L312, "ConductorOne download center"). Not changed by this PR.
Cuts the sync from one Classic detail read per item (and per role and per site) to a handful of paged Jamf Pro API calls. Mock tenant: 2,125 requests -> 31. Trial tenant (1,114 users, 22 Custom accounts): 50 requests, 43 s, 0 errors. Also fixes CXP-1286 (users shown as
user) and resolves CXP-1285 (duplicate of CXP-1240).What changed
Read Account Groups).roleis type-scoped (TypeScopedGrants): one pass over the accounts, then the Custom accounts at 10 per page with one detail read each. Group role grants are emitted fromgroupGrants.group,userGroup,siteandroleuseStaticEntitlements+SkipEntitlements.userGroupGrants read the group once; later pages come from the session store (1000 member ids per key).FailedPrecondition, notNotFound: aNotFoundfrom List/Grants makes the SDK skip the action and C1 deletes the data. Requires Jamf Pro 11.26.0 or later (README, connector.mdx; docs-info lists each endpoint's version).InvalidArgumenton bad CreateAccount/Delete input, Delete refuses the connector's own account,userAccountdisplay name fromFullName, site errors with context,getUserIndexlock removed with the index.Visible in C1 (no ID changes)
userAccountdisplay names show the full name; roles gain a description.Read Useror user group reads, and managed devices no longer needRead User.managedDevice-only sync now emits device grants withExternalResourceMatch(main emitted none after the SDK ingest filter).Deviations from the ticket
Validateis unchanged (token check only): the extra read was dropped.Known, pre-existing, deferred to CXP-1241 (device work; ships with this before CXP-1217)
deviceOwnersis an in-process map filled in List: on Lambda, Entitlements/Grants run in other invokes and emit nothing. Fix: assignee on the device profile.grant.NewGrantwith a nil id (panic).NotFoundon 404; per-device lookup whenuseris not synced; always emitassigned.