SAAD: Asset CRUD API - #49011
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the first slice of the Apple DDM “asset” CRUD API to Fleet, wiring up new REST routes, authorization policy, service/datastore interfaces, MySQL datastore implementations, and initial test coverage. This aligns with issue #48568’s “Storage + REST API” portion (the PR is explicitly marked as partial).
Changes:
- Register new
/api/v1/fleet/assetsendpoints (list/get/download viaalt=media/create/delete) and add OSS stubs returningErrMissingLicense. - Introduce new Fleet service/datastore interfaces + MySQL CRUD implementations and supporting Fleet types for DDM assets.
- Add authz policy + tests, plus datastore and EE service tests for the new asset operations.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/handler.go | Registers new asset CRUD routes under the Apple MDM middleware. |
| server/service/apple_mdm.go | Adds request/response structs and endpoint handlers; OSS service methods stubbed for premium gating. |
| server/mock/service/service_mock.go | Extends service mock with the new DDM asset methods. |
| server/mock/datastore_mock.go | Extends datastore mock with the new DDM asset methods. |
| server/fleet/service.go | Adds DDM asset methods to the Fleet Service interface. |
| server/fleet/request.go | Introduces MaxMDMAssetSize request size constant. |
| server/fleet/datastore.go | Adds DDM asset CRUD methods to the datastore interface. |
| server/fleet/apple_mdm.go | Introduces DDMAsset* structs and authz wrapper type for DDM assets. |
| server/datastore/mysql/apple_mdm.go | Implements MySQL CRUD for DDM assets. |
| server/datastore/mysql/apple_mdm_test.go | Adds unit tests for the new MySQL DDM asset CRUD methods. |
| server/authz/policy.rego | Adds read/write policy rules for ddm_asset objects. |
| server/authz/policy_test.go | Adds test coverage for the new ddm_asset authorization rules. |
| server/api_endpoints/api_endpoints.yml | Documents the new public API endpoints. |
| ee/server/service/apple_mdm.go | Implements the real (EE) service logic for listing/getting/downloading/creating/deleting assets. |
| ee/server/service/apple_mdm_test.go | Adds initial EE service-layer tests for list/get/download/delete authorization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis change adds Apple DDM asset support across authorization, datastore CRUD, service interfaces, HTTP handlers, mocks, and tests. It introduces DDM asset types and authz typing, adds Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/datastore/mysql/apple_mdm_test.go (1)
55-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage — consider adding a regression test for the non-duplicate insert-error path.
Coverage for validation, duplicate-conflict, and FK-violation paths is thorough. One gap: there's no test forcing a non-duplicate DB error on
CreateAppleDDMAsset's insert (e.g. via a bad column/type or a mocked writer failure), which is exactly the scenario affected by the error-swallowing bug flagged inserver/datastore/mysql/apple_mdm.go. Adding one would lock in the fix once applied.Also applies to: 13295-13495
🤖 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_test.go` around lines 55 - 59, Add a regression test for CreateAppleDDMAsset that forces a non-duplicate insert failure on the initial DB insert path, not a validation, duplicate-key, or FK error. Use the existing apple_mdm_test.go test table and the CreateAppleDDMAsset flow to simulate a generic writer/DB failure (for example by mocking the inserter or using an invalid write scenario), then assert the error is returned instead of being swallowed. This will protect the fix in apple_mdm.go and cover the missing non-duplicate error path.ee/server/service/apple_mdm_test.go (1)
1-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
CreateAppleDDMAsset/validateAppleDDMAsset.This file tests List/Get/Download/Delete thoroughly but has no test for
CreateAppleDDMAsset, which is the most complex path (authz, JSON parsing, secret expansion, and duplicate-name/identifier conflict mapping). Given the data-flow concern raised inapple_mdm.goaround secret expansion at create time, a test asserting the persisted/created payload reflects the expected (expanded) data would materially reduce risk here.🤖 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 `@ee/server/service/apple_mdm_test.go` around lines 1 - 348, Add test coverage for the CreateAppleDDMAsset path, since apple_mdm_test.go currently only exercises List/Get/Download/Delete. Create a focused test around CreateAppleDDMAsset that verifies authz handling plus validateAppleDDMAsset behavior, including JSON parsing, secret expansion, and duplicate-name/identifier conflict mapping; use the existing service method names and mock.Store hooks to assert the created payload is the expanded/persisted version.
🤖 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.
Inline comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 7852-7897: The CreateAppleDDMAsset insert path is swallowing
non-duplicate errors and then returning assetUUID as if the row was created
successfully. Update the error handling around ds.writer(ctx).ExecContext in
CreateAppleDDMAsset so any non-nil err that is not handled as a duplicate also
returns a wrapped error instead of falling through. Keep the existing
duplicate-key mapping logic for IsDuplicate(err), but add a fallback return in
the err block so callers never receive a fake success.
In `@server/service/apple_mdm.go`:
- Around line 3786-3796: The multipart form parsing in the fleet_id handling
path currently accepts negative values and then converts them to uint, which can
silently wrap to a huge TeamID. Update the fleet_id validation in the
apple_mdm.go multipart decode logic to reject any parsed value less than zero
before assigning to decoded.TeamID, and return the same BadRequestError used for
other invalid fleet_id inputs. Keep the fix localized to the fleet_id parsing
block that uses strconv.Atoi and decoded.TeamID.
---
Nitpick comments:
In `@ee/server/service/apple_mdm_test.go`:
- Around line 1-348: Add test coverage for the CreateAppleDDMAsset path, since
apple_mdm_test.go currently only exercises List/Get/Download/Delete. Create a
focused test around CreateAppleDDMAsset that verifies authz handling plus
validateAppleDDMAsset behavior, including JSON parsing, secret expansion, and
duplicate-name/identifier conflict mapping; use the existing service method
names and mock.Store hooks to assert the created payload is the
expanded/persisted version.
In `@server/datastore/mysql/apple_mdm_test.go`:
- Around line 55-59: Add a regression test for CreateAppleDDMAsset that forces a
non-duplicate insert failure on the initial DB insert path, not a validation,
duplicate-key, or FK error. Use the existing apple_mdm_test.go test table and
the CreateAppleDDMAsset flow to simulate a generic writer/DB failure (for
example by mocking the inserter or using an invalid write scenario), then assert
the error is returned instead of being swallowed. This will protect the fix in
apple_mdm.go and cover the missing non-duplicate error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc85185c-89bd-41d5-8b25-b910d115b91a
📒 Files selected for processing (15)
ee/server/service/apple_mdm.goee/server/service/apple_mdm_test.goserver/api_endpoints/api_endpoints.ymlserver/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/request.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/handler.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49011 +/- ##
==========================================
+ Coverage 68.08% 68.09% +0.01%
==========================================
Files 3709 3709
Lines 235098 235603 +505
Branches 12353 12353
==========================================
+ Hits 160073 160441 +368
- Misses 60637 60757 +120
- Partials 14388 14405 +17
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ee/server/service/apple_mdm_test.go (2)
351-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
AlreadyExistsError→ConflictErrortranslation.The service implementation translates
AlreadyExistsErrorintoConflictErrorfor both"asset_name"and"asset_identifier"cases (ee/server/service/apple_mdm.go:150-157), but no subtest exercises this path. This is important business logic that should be tested to prevent regressions.🧪 Suggested conflict translation subtests
t.Run("Expanded payload with authentication key is rejected", func(t *testing.T) { // ... existing test ... }) + + t.Run("Duplicate name returns conflict error", func(t *testing.T) { + defer reset() + ds.CreateAppleDDMAssetFunc = func(ctx context.Context, name, identifier string, data []byte, teamID *uint) (string, error) { + return "", fleet.AlreadyExistsError{Message: "asset_name: asset already exists for team"} + } + _, err := svc.CreateAppleDDMAsset(ctx, nil, "asset", validData) + require.Error(t, err) + var conflictErr *fleet.ConflictError + require.ErrorAs(t, err, &conflictErr) + require.Contains(t, conflictErr.Error(), "already exists for this team") + }) + + t.Run("Duplicate identifier returns conflict error", func(t *testing.T) { + defer reset() + ds.CreateAppleDDMAssetFunc = func(ctx context.Context, name, identifier string, data []byte, teamID *uint) (string, error) { + return "", fleet.AlreadyExistsError{Message: "asset_identifier: identifier already exists for team"} + } + _, err := svc.CreateAppleDDMAsset(ctx, nil, "asset", validData) + require.Error(t, err) + var conflictErr *fleet.ConflictError + require.ErrorAs(t, err, &conflictErr) + require.Contains(t, conflictErr.Error(), "already exists for this team") + }) }🤖 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 `@ee/server/service/apple_mdm_test.go` around lines 351 - 503, Add subtest coverage in TestCreateAppleDDMAsset for the AlreadyExistsError to ConflictError translation path. Update the mock Store.CreateAppleDDMAssetFunc to return an AlreadyExistsError for both the asset_name and asset_identifier cases, then assert CreateAppleDDMAsset returns a ConflictError in each scenario. Use the existing svc.CreateAppleDDMAsset test harness and the mock.Store symbols to keep the new cases aligned with the service’s conflict handling logic.
421-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert
ExpandEmbeddedSecretsAndUpdatedAtFuncInvokedis false in field-validation subtests for consistency.The "Malformed JSON" and secret-in-type/identifier subtests assert
ds.ExpandEmbeddedSecretsAndUpdatedAtFuncInvokedisfalse, but the empty-identifier, invalid-type, empty-DataURL, and invalid-DataURL subtests only assertds.CreateAppleDDMAssetFuncInvokedisfalse. Adding the expansion assertion here too would confirm that field validation gates expansion consistently and protect against future reordering.🤖 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 `@ee/server/service/apple_mdm_test.go` around lines 421 - 451, The field-validation subtests in CreateAppleDDMAssetTest only verify that ds.CreateAppleDDMAssetFuncInvoked stays false, but they should also assert ds.ExpandEmbeddedSecretsAndUpdatedAtFuncInvoked is false for consistency with the other validation cases. Update the empty-identifier, invalid-asset-type, empty-DataURL, and invalid-DataURL subtests in apple_mdm_test.go to include the same expansion-not-invoked assertion used in the Malformed JSON and secret-related cases.
🤖 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.
Nitpick comments:
In `@ee/server/service/apple_mdm_test.go`:
- Around line 351-503: Add subtest coverage in TestCreateAppleDDMAsset for the
AlreadyExistsError to ConflictError translation path. Update the mock
Store.CreateAppleDDMAssetFunc to return an AlreadyExistsError for both the
asset_name and asset_identifier cases, then assert CreateAppleDDMAsset returns a
ConflictError in each scenario. Use the existing svc.CreateAppleDDMAsset test
harness and the mock.Store symbols to keep the new cases aligned with the
service’s conflict handling logic.
- Around line 421-451: The field-validation subtests in CreateAppleDDMAssetTest
only verify that ds.CreateAppleDDMAssetFuncInvoked stays false, but they should
also assert ds.ExpandEmbeddedSecretsAndUpdatedAtFuncInvoked is false for
consistency with the other validation cases. Update the empty-identifier,
invalid-asset-type, empty-DataURL, and invalid-DataURL subtests in
apple_mdm_test.go to include the same expansion-not-invoked assertion used in
the Malformed JSON and secret-related cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffdf76e4-3460-4142-b82f-61caa7e1ddb1
📒 Files selected for processing (1)
ee/server/service/apple_mdm_test.go
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/datastore/mysql/apple_mdm_ddm_test.go (1)
444-470: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting returned asset fields in the "returns asset" subtest.
The "returns asset for existing asset" subtest only checks
require.NotNil(t, asset)but doesn't verify any field values (e.g.,asset.Name,asset.Identifier). The download variant at lines 481-492 does assert field values. Adding basic field assertions here would strengthen coverage and catch silent column-mapping regressions.🤖 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_ddm_test.go` around lines 444 - 470, The "returns asset for existing asset" subtest in testGetAppleDDMAsset only checks that GetAppleDDMAsset returns a non-nil result, so extend it to assert the returned AppleDDMAsset fields match the values passed to CreateAppleDDMAsset, such as Name and Identifier. Use the existing GetAppleDDMAsset and CreateAppleDDMAsset calls in that subtest as the anchor, and mirror the field-level coverage already used in the download variant to catch mapping regressions.
🤖 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.
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_ddm_test.go`:
- Around line 444-470: The "returns asset for existing asset" subtest in
testGetAppleDDMAsset only checks that GetAppleDDMAsset returns a non-nil result,
so extend it to assert the returned AppleDDMAsset fields match the values passed
to CreateAppleDDMAsset, such as Name and Identifier. Use the existing
GetAppleDDMAsset and CreateAppleDDMAsset calls in that subtest as the anchor,
and mirror the field-level coverage already used in the download variant to
catch mapping regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af128f0f-60fb-4860-96d3-80a35be6387c
📒 Files selected for processing (6)
ee/server/service/apple_mdm.goserver/api_endpoints/api_endpoints.ymlserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/service/apple_mdm.goserver/service/handler.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/apple_mdm.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/api_endpoints/api_endpoints.yml
- server/service/handler.go
- server/service/apple_mdm.go
- ee/server/service/apple_mdm.go
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
I haven't tested this yet but overall code looks good and I am just about to test with the frontend so no concerns
Related issue: Resolves #48568 partly
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. (Will add in followup)
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
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit