BYOF: API & GitOps support - #47506
Conversation
76a442a to
45bdf72
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47506 +/- ##
==========================================
+ Coverage 67.19% 67.20% +0.01%
==========================================
Files 3621 3622 +1
Lines 229299 229507 +208
Branches 11762 11762
==========================================
+ Hits 154071 154245 +174
- Misses 61372 61399 +27
- Partials 13856 13863 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a08d81c to
5c75f4d
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds BYOD fleet/team support to Apple Business Manager (ABM) token assignments across the Fleet API and GitOps, extending existing macOS/iOS/iPadOS assignment plumbing to include a BYOD default team and the associated config/key rename support.
Changes:
- Extend ABM token/team models and update ABM token teams API endpoint to accept/return BYOD team assignments.
- Persist and hydrate BYOD default team in MySQL datastore queries and update datastore tests accordingly.
- Add GitOps support for
byod_team/byod_fleetkeys, updating fleetctl generate/apply fixtures and tests.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/cloner-check/generated_files/appconfig.txt | Updates generated appconfig type field list to include BYODTeam for ABM assignment info. |
| server/service/integration_mdm_test.go | Adjusts service discovery integration test coverage for BYOD enrollment URLs (contains a duplicate block to remove). |
| server/service/apple_mdm.go | Extends UpdateABMTokenTeams request/endpoint wiring to include byod_team_id. |
| server/service/appconfig.go | Extends ABM assignment validation/application to include BYOD team (needs BYOD reset + normalization fix). |
| server/mock/service/service_mock.go | Updates service mock interface to match new UpdateABMTokenTeams signature. |
| server/fleet/service.go | Updates Service interface contract/docs for BYOD team support in UpdateABMTokenTeams. |
| server/fleet/mdm.go | Extends ABMToken model with BYOD default team ID, DB join field, and API response field. |
| server/fleet/app.go | Extends MDMAppleABMAssignmentInfo with BYODTeam for GitOps/API config. |
| server/datastore/mysql/apple_mdm.go | Adds BYOD default team to ABM token insert/update/select queries and hydrates BYODTeam in responses. |
| server/datastore/mysql/apple_mdm_test.go | Extends ABM token datastore tests to validate BYOD team persistence/hydration. |
| pkg/spec/gitops_deprecations.go | Adds GitOps deprecated key mapping for byod_team -> byod_fleet. |
| ee/server/service/mdm.go | Implements BYOD team handling in EE UpdateABMTokenTeams (validation + persistence). |
| cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.yml | Updates GitOps fixture to include byod_fleet. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yaml | Updates expected GitOps output to include byod_fleet. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yaml | Updates expected insecure GitOps output to include byod_fleet. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/appConfig.json | Updates app config JSON fixture to include byod_team. |
| cmd/fleetctl/fleetctl/gitops_test.go | Extends GitOps tests to cover BYOD team assignment and renamed key support. |
| docs/Contributing/reference/api-for-contributors.md | Updated contributor API reference (content excluded from review per policy). |
Files excluded by content exclusion policy (1)
- docs/Contributing/reference/api-for-contributors.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review once |
|
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds BYOD (Bring Your Own Device) team assignment support to Apple Business Manager (ABM) tokens. New Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/datastore/mysql/apple_mdm.go (1)
6167-6171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude BYOD defaults in ABM token team association lookups.
This helper now misses tokens where the team is associated only through
byod_default_team_id, including the No Team case when only the BYOD default is unset.Proposed fix
- abmtFilter := `abmt.macos_default_team_id IS NULL OR abmt.ios_default_team_id IS NULL OR abmt.ipados_default_team_id IS NULL` + abmtFilter := `abmt.macos_default_team_id IS NULL OR abmt.ios_default_team_id IS NULL OR abmt.ipados_default_team_id IS NULL OR abmt.byod_default_team_id IS NULL` if teamID != nil { teamFilter = `h.team_id = ?` - abmtFilter = `abmt.macos_default_team_id = ? OR abmt.ios_default_team_id = ? OR abmt.ipados_default_team_id = ?` - args = append(args, *teamID, *teamID, *teamID, *teamID) + abmtFilter = `abmt.macos_default_team_id = ? OR abmt.ios_default_team_id = ? OR abmt.ipados_default_team_id = ? OR abmt.byod_default_team_id = ?` + args = append(args, *teamID, *teamID, *teamID, *teamID, *teamID) }As per coding guidelines, SQL queries intended to return data for a specific entity must apply precise filtering criteria.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 6167 - 6171, The ABM token team association filters in the helper are incomplete because they do not account for the byod_default_team_id field. Add byod_default_team_id to both filter conditions: in the first abmtFilter (when teamID is nil), include OR abmt.byod_default_team_id IS NULL, and in the second abmtFilter (when teamID is not nil), include OR abmt.byod_default_team_id = ?. Also update the args slice to append *teamID one additional time to provide the parameter value for the byod_default_team_id condition in the teamID case.Source: Coding guidelines
server/service/appconfig.go (1)
2014-2031:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize team-name keys consistently before assignment lookup.
Line 2015 validates using
norm.NFC.String(tmName), but Lines 2027-2030 assign using raw team-name keys. This can pass validation and still resolve tonilon assignment when normalization forms differ, silently writing wrong default-team IDs (including BYOD).Suggested fix
- teamsByName := map[string]*uint{"": nil, "No team": nil} + teamsByName := map[string]*uint{ + norm.NFC.String(""): nil, + norm.NFC.String("No team"): nil, + } for _, tm := range teams { - teamsByName[tm.Name] = &tm.ID + teamsByName[norm.NFC.String(tm.Name)] = &tm.ID } @@ - tok := tokensByName[bm.OrganizationName] - tok.MacOSDefaultTeamID = teamsByName[bm.MacOSTeam] - tok.IOSDefaultTeamID = teamsByName[bm.IOSTeam] - tok.IPadOSDefaultTeamID = teamsByName[bm.IpadOSTeam] - tok.BYODDefaultTeamID = teamsByName[bm.BYODTeam] + tok := tokensByName[norm.NFC.String(bm.OrganizationName)] + tok.MacOSDefaultTeamID = teamsByName[norm.NFC.String(bm.MacOSTeam)] + tok.IOSDefaultTeamID = teamsByName[norm.NFC.String(bm.IOSTeam)] + tok.IPadOSDefaultTeamID = teamsByName[norm.NFC.String(bm.IpadOSTeam)] + tok.BYODDefaultTeamID = teamsByName[norm.NFC.String(bm.BYODTeam)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/appconfig.go` around lines 2014 - 2031, The code validates team names using normalized form `norm.NFC.String(tmName)` to check against the `teamsByName` map, but then performs lookups using raw non-normalized team names when assigning default team IDs to the token object. This inconsistency causes validation to pass while assignment silently fails with nil values. Apply `norm.NFC.String()` normalization to the team name keys when accessing `teamsByName` during the assignment of `tok.MacOSDefaultTeamID`, `tok.IOSDefaultTeamID`, `tok.IPadOSDefaultTeamID`, and `tok.BYODDefaultTeamID` to ensure consistent normalization throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 6167-6171: The ABM token team association filters in the helper
are incomplete because they do not account for the byod_default_team_id field.
Add byod_default_team_id to both filter conditions: in the first abmtFilter
(when teamID is nil), include OR abmt.byod_default_team_id IS NULL, and in the
second abmtFilter (when teamID is not nil), include OR abmt.byod_default_team_id
= ?. Also update the args slice to append *teamID one additional time to provide
the parameter value for the byod_default_team_id condition in the teamID case.
In `@server/service/appconfig.go`:
- Around line 2014-2031: The code validates team names using normalized form
`norm.NFC.String(tmName)` to check against the `teamsByName` map, but then
performs lookups using raw non-normalized team names when assigning default team
IDs to the token object. This inconsistency causes validation to pass while
assignment silently fails with nil values. Apply `norm.NFC.String()`
normalization to the team name keys when accessing `teamsByName` during the
assignment of `tok.MacOSDefaultTeamID`, `tok.IOSDefaultTeamID`,
`tok.IPadOSDefaultTeamID`, and `tok.BYODDefaultTeamID` to ensure consistent
normalization throughout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f290a190-13c7-4ba5-8819-a056c1550b4c
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/api-for-contributors.mdis excluded by!**/*.md
📒 Files selected for processing (16)
cmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/generateGitops/appConfig.jsoncmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.ymlee/server/service/mdm.gopkg/spec/gitops_deprecations.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/app.goserver/fleet/mdm.goserver/fleet/service.goserver/mock/service/service_mock.goserver/service/appconfig.goserver/service/apple_mdm.gotools/cloner-check/generated_files/appconfig.txt
|
@rachaelshaw Let me know if we are not okay with this, and would rather pull it out and target for |
Related issue: Resolves #45600
I could see the contributor endpoint was not updated, so I just included it in this PR, and since it's a contributor one I think we are fine updating ahead of release.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information. (Part of previous PR)
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
Release Notes
New Features
Refactor
byod_fleet, with migration/aliasing from the olderbyod_teamkey.Tests