Add team assignment checks to APIs that do label association#37246
Add team assignment checks to APIs that do label association#37246
Conversation
Co-authored-by: Ian Littman <iansltx@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR adds team-aware label validation to prevent mismatched label-entity associations. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/testing_client.go (1)
143-161: Teardown cleanup scope/comment mismatch: unscoped DELETEs forin_house_apps/vpp_apps.If the intent is to clean only “No team”, mirror the
global_or_team_id = 0pattern (if those tables have it). If the intent is “wipe all”, please update the comment to avoid misleading future changes.- // Clean software installers in "No team" (the others are deleted in ts.ds.DeleteTeam above). + // Clean "No team" software installers; also cleanup app tables for test isolation.
🧹 Nitpick comments (8)
server/fleet/service.go (1)
278-285: ClarifyteamIDsemantics (nilvs0) in the interface docstring.
verifyLabelsToAssociatetreats bothnil(global) and0(“no team”) as “labels must be global”, but that nuance isn’t obvious from “belong to the given teamID.” Consider documenting thenil/0behavior explicitly.server/service/labels_util.go (1)
24-72: Team/global/no-team association rules are enforced as intended; add targeted mismatch tests.
TheentityTeamID == nil || *entityTeamID == 0branch + “same-team or global” rule for team entities matches the stated constraints; I’d add a small set of unit tests covering the negative paths to lock behavior.server/service/labels.go (1)
683-720: Good:BatchValidateLabelsnow enforces team-aware association after existence validation.
Authz precondition is preserved, and the newverifyLabelsToAssociatestep aligns with the PR goal. (Optional micro-optimization: passuniqueNamesinstead oflabelNames, sinceverifyLabelsToAssociatededupes anyway.)server/service/labels_test.go (1)
378-387: Tests are correctly updated to account for team-aware validation (newLabelsByNameFuncmock).
Consider extendingTestBatchValidateLabelswith cases whereteamIDis non-nil and a label has a differentTeamID, and whereteamIDisnil/0and a label has a non-nilTeamID(expect error).Also applies to: 416-430, 482-483
server/service/integration_mdm_test.go (3)
19264-19269: Minor inconsistency:globalLabelomits explicit type fields.For consistency with the other label creations in this test (and hunk 1), consider adding the explicit
LabelTypeandLabelMembershipTypefields. While it works correctly due to Go's zero-value defaults matching the iota definitions, explicit fields improve readability.globalLabel, err := s.ds.NewLabel(t.Context(), &fleet.Label{ - Name: "global", - Query: "SELECT global;", - TeamID: nil, + Name: "global", + Query: "SELECT global;", + TeamID: nil, + LabelType: fleet.LabelTypeRegular, + LabelMembershipType: fleet.LabelMembershipTypeDynamic, })
19408-19450: Fix comment numbering inconsistencies in "No team" policy section.The comments in this section use incorrect subsection numbers:
- Line 19408: "1.B.2" should be "1.C.2"
- Line 19422: "1.B.3" should be "1.C.3"
- Line 19437: "1.B.3" should be "1.C.4" and "team policy" should be "No team policy"
- // 1.B.2 Attempt to create a "No team" policy with a global label (should succeed). + // 1.C.2 Attempt to create a "No team" policy with a global label (should succeed). tpResp = teamPolicyResponse{} s.DoJSON("POST", "/api/latest/fleet/teams/0/policies", teamPolicyRequest{- // 1.B.3 Attempt to edit a "No team" policy with a team policy that references l2t2 (should fail). + // 1.C.3 Attempt to edit a "No team" policy with labels that reference l2t2 (should fail). mtplr = modifyTeamPolicyResponse{}- // 1.B.3 Attempt to edit a team policy with a team policy that references a global label (should succeed). + // 1.C.4 Attempt to edit a "No team" policy with a global label (should succeed). mtplr = modifyTeamPolicyResponse{}
19510-19526: Fix misleading variable name and comment numbering.
- Variable
team1LabelIDstores a query ID, not a label ID - rename toteam1QueryIDfor clarity.- Comments on lines 19512 and 19520 use "2.A.x" numbering but should use "2.B.x" since they're in the team query subsection.
s.DoJSON("POST", "/api/latest/fleet/queries", reqQuery, http.StatusOK, &createQueryResp) - team1LabelID := createQueryResp.Query.ID + team1QueryID := createQueryResp.Query.ID - // 2.A.3 Attempt to edit a team query with a label of another team (should fail). + // 2.B.3 Attempt to edit a team query with a label of another team (should fail). modifyQueryResp = modifyQueryResponse{} - s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/queries/%d", team1LabelID), modifyQueryRequest{ + s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/queries/%d", team1QueryID), modifyQueryRequest{ QueryPayload: fleet.QueryPayload{ LabelsIncludeAny: []string{l2t2.Name, globalLabel.Name}, }, }, http.StatusBadRequest, &modifyQueryResp) - // 2.A.4 Attempt to edit team query with a label of the same team (should succeed). + // 2.B.4 Attempt to edit team query with a label of the same team (should succeed). modifyQueryResp = modifyQueryResponse{} - s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/queries/%d", team1LabelID), modifyQueryRequest{ + s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/queries/%d", team1QueryID), modifyQueryRequest{server/service/software_installers_test.go (1)
213-342: Consider adding test coverage for team-specific label validation.The tests correctly pass
nilfor the newteamIDparameter, covering the global/no-team scenario. However, given that the PR objective is to add "team assignment checks" to enforce team consistency and prevent cross-team label assignment, consider adding tests that verify:
- Labels from a specific team can be successfully validated when the correct
teamIDis provided- Labels from team A are rejected when attempting to associate with an entity in team B
- Global labels (teamID=nil) behavior when used with team entities
- Appropriate error messages are returned for team mismatches
This would ensure the security feature works as intended and provide regression protection.
Generate a test case example:
t.Run("reject labels from different team", func(t *testing.T) { teamID := uint(1) teamLabels := map[string]uint{ "team1-label": 10, } ds.LabelIDsByNameFunc = func(ctx context.Context, names []string) (map[string]uint, error) { // Return the labels but they should be rejected due to team mismatch return teamLabels, nil } // Attempt to validate team 1 labels for team 2 entity differentTeamID := uint(2) _, err := eeservice.ValidateSoftwareLabels(ctx, svc, &differentTeamID, []string{"team1-label"}, nil) require.Error(t, err) require.Contains(t, err.Error(), "team") // or whatever the expected error message is })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/fleetctl/fleetctl/gitops_test.go(1 hunks)cmd/fleetctl/integrationtest/gitops/software_test.go(2 hunks)ee/server/service/maintained_apps.go(1 hunks)ee/server/service/software_installers.go(5 hunks)ee/server/service/vpp.go(3 hunks)server/fleet/service.go(1 hunks)server/mock/service/service_mock.go(2 hunks)server/service/apple_mdm.go(1 hunks)server/service/global_policies.go(1 hunks)server/service/integration_mdm_test.go(2 hunks)server/service/labels.go(2 hunks)server/service/labels_test.go(3 hunks)server/service/labels_util.go(1 hunks)server/service/mdm.go(6 hunks)server/service/mdm_test.go(1 hunks)server/service/queries.go(2 hunks)server/service/software_installers_test.go(2 hunks)server/service/team_policies.go(2 hunks)server/service/testing_client.go(1 hunks)server/service/windows_mdm_profiles.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/testing_client.goserver/service/integration_mdm_test.goserver/service/windows_mdm_profiles.goserver/service/labels_util.goserver/service/team_policies.goserver/service/queries.goee/server/service/maintained_apps.goee/server/service/software_installers.goserver/mock/service/service_mock.goserver/service/global_policies.goserver/fleet/service.goee/server/service/vpp.gocmd/fleetctl/integrationtest/gitops/software_test.goserver/service/apple_mdm.goserver/service/labels_test.goserver/service/software_installers_test.goserver/service/labels.goserver/service/mdm.goserver/service/mdm_test.gocmd/fleetctl/fleetctl/gitops_test.go
🧠 Learnings (6)
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
server/service/testing_client.go
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/service/integration_mdm_test.goserver/service/apple_mdm.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/service/integration_mdm_test.goserver/service/labels_test.goserver/service/software_installers_test.gocmd/fleetctl/fleetctl/gitops_test.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/service/windows_mdm_profiles.goserver/service/apple_mdm.goserver/service/mdm.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
server/service/labels_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/service/labels_test.gocmd/fleetctl/fleetctl/gitops_test.go
🧬 Code graph analysis (16)
server/service/integration_mdm_test.go (2)
server/fleet/labels.go (2)
LabelTypeRegular(65-65)LabelMembershipTypeDynamic(100-100)server/fleet/software_installer.go (1)
UploadSoftwareInstallerPayload(489-525)
server/service/labels_util.go (1)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/service/team_policies.go (2)
server/fleet/mdm.go (2)
LabelsIncludeAny(868-868)LabelsExcludeAny(869-869)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/service/queries.go (2)
server/fleet/mdm.go (1)
LabelsIncludeAny(868-868)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
ee/server/service/maintained_apps.go (1)
ee/server/service/software_installers.go (1)
ValidateSoftwareLabels(196-230)
ee/server/service/software_installers.go (2)
server/fleet/service.go (1)
Service(75-1400)server/fleet/labels.go (1)
LabelIdentsWithScope(302-305)
server/mock/service/service_mock.go (2)
server/fleet/labels.go (1)
LabelIdent(281-284)server/fleet/service.go (1)
Service(75-1400)
server/service/global_policies.go (2)
server/fleet/mdm.go (2)
LabelsIncludeAny(868-868)LabelsExcludeAny(869-869)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/fleet/service.go (1)
server/fleet/labels.go (1)
LabelIdent(281-284)
ee/server/service/vpp.go (2)
ee/server/service/software_installers.go (1)
ValidateSoftwareLabels(196-230)server/fleet/mdm.go (2)
LabelsIncludeAny(868-868)LabelsExcludeAny(869-869)
cmd/fleetctl/integrationtest/gitops/software_test.go (1)
server/mock/datastore_mock.go (1)
LabelsByNameFunc(180-180)
server/service/labels_test.go (2)
server/contexts/authz/authz.go (1)
AuthorizationContext(62-68)server/mock/datastore_mock.go (1)
LabelsByNameFunc(180-180)
server/service/software_installers_test.go (2)
ee/server/service/software_installers.go (1)
ValidateSoftwareLabels(196-230)server/contexts/authz/authz.go (1)
AuthorizationContext(62-68)
server/service/labels.go (3)
server/fleet/service.go (1)
Service(75-1400)server/fleet/labels.go (1)
LabelIdent(281-284)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/service/mdm_test.go (1)
server/mock/datastore_mock.go (1)
LabelsByNameFunc(180-180)
cmd/fleetctl/fleetctl/gitops_test.go (2)
server/mock/datastore_mock.go (1)
LabelsByNameFunc(180-180)pkg/spec/gitops.go (1)
Label(159-162)
🔇 Additional comments (21)
cmd/fleetctl/fleetctl/gitops_test.go (1)
2678-2691: LGTM! Mock implementation correctly provides label objects for testing.The
LabelsByNameFuncmock properly complements the existingLabelIDsByNameFuncby returning fullfleet.Labelobjects. The implementation correctly handles the lookup logic and returns only labels that exist in thelabelToIDsmap, which is appropriate for test mocking.cmd/fleetctl/integrationtest/gitops/software_test.go (2)
554-565: LGTM! Label mock correctly resolves label names to Label objects.The
LabelsByNameFuncmock implementation properly mirrors the existingLabelIDsByNameFuncpattern and returns fullfleet.Labelobjects for the test cases. The logic correctly filters based onc.expectedLabels.
580-580: LGTM! Assertion correctly verifies label function invocation.The assertion appropriately checks that
LabelsByNameFuncwas invoked when expected labels are present, aligning with the existing verification pattern forLabelIDsByNameFunc.ee/server/service/vpp.go (1)
149-149: LGTM! VPP label validation now correctly includes team scope.The updated calls to
ValidateSoftwareLabelsproperly pass theteamIDparameter, enabling team-aware label validation for VPP apps. This correctly enforces the team consistency checks described in the PR objectives for app store apps across batch association, adding, and updating operations.Also applies to: 484-484, 764-764
server/mock/service/service_mock.go (2)
165-166: Signature update matches interface; good passthrough shape.
2654-2659: BatchValidateLabels correctly forwards teamID to the mock func.server/service/windows_mdm_profiles.go (1)
64-67: Correctly threads team context into label validation.server/service/mdm_test.go (1)
2363-2375: LGTM! Clean test mock implementation.The
LabelsByNameFuncmock correctly maps label names to fullLabelobjects, supporting test scenarios that need label resolution. The "baddy" exclusion enables negative test cases.ee/server/service/maintained_apps.go (1)
43-43: LGTM! Correct team context for label validation.The updated call to
ValidateSoftwareLabelsnow includesteamID, ensuring labels are validated against the appropriate team scope before creating the maintained app installer.server/service/queries.go (2)
284-286: LGTM! Appropriate label validation in query creation.The validation correctly uses
p.TeamID(from the payload) and checks labels before creating the query entity. This ensures labels exist and match the team scope.
424-427: LGTM! Correct use of existing query's team context.The comment clarifies why
query.TeamID(not payload) is used—teams cannot be changed during modification. This ensures label validation uses the correct team scope.server/service/team_policies.go (2)
94-96: LGTM! Comprehensive label validation for policy creation.The validation correctly combines both
LabelsIncludeAnyandLabelsExcludeAnyto verify all labels belong to the appropriate team scope before creating the policy.
573-575: LGTM! Comprehensive label validation for policy modification.The validation ensures all labels (both include and exclude) are verified against the policy's team context before applying modifications.
server/service/apple_mdm.go (1)
439-452: No issues found with the label validation logic for no-team profiles. The code correctly passes&teamIDtovalidateProfileLabels, and the underlyingverifyLabelsToAssociatefunction explicitly handles bothniland*uint(0)identically, treating both as entities that must use only global labels. This pattern is already used elsewhere in the method (line 430) and is validated by integration tests covering no-team profiles with labels.server/service/labels_util.go (1)
10-22:loadLabelsFromNamescorrectness looks good (explicit not-found validation).
The post-check ensures partial results fromLabelsByNamecan’t silently pass.ee/server/service/software_installers.go (2)
54-60: Good: software label validation is now team-aware by construction (threadsteamIDintoBatchValidateLabels).
This matches the “assignment checks” objective and keeps the enforcement centralized in label validation.Also applies to: 196-230
713-716: All call sites forValidateSoftwareLabelsandBatchValidateLabelsare properly updated. Verification confirms all 9 call sites ofValidateSoftwareLabelsuse the correct 5-parameter signature (ctx, svc, teamID, labelsIncludeAny, labelsExcludeAny), and the single production call site ofBatchValidateLabelsuses the correct 3-parameter signature (ctx, teamID, labelNames). No stale call sites remain.server/service/integration_mdm_test.go (2)
19079-19104: LGTM!The label creation setup is consistent, with explicit
LabelTypeandLabelMembershipTypefields for all three labels (team-scoped and global).
19529-19765: LGTM!The remaining test sections (configuration profiles, software installers, VPP apps, in-house apps) comprehensively cover both positive and negative test cases for team-aware label validation. The final cleanup verifying team deletion succeeds is a good safeguard to ensure no foreign key issues arise from the test data.
server/service/mdm.go (2)
1968-2021: LGTM: Team context properly propagated through batch operations.The signature update to
BatchSetMDMProfilescorrectly introducestmID *uintandtmName *stringparameters, and the team ID is properly passed through tobatchValidateProfileLabelsat line 2016. This ensures labels are validated against the correct team scope before profiles are batch-set.The changes align with the PR objective to enforce team consistency in label associations.
1817-1856: The team-scoped label validation inverifyLabelsToAssociateis correctly implemented. The function properly enforces team consistency: it rejects team labels for "no team" entities, allows global labels for any entity, and prevents team entities from referencing labels that belong to other teams. BothLabelIDsByNameandLabelsByNameuse safe SQL parameter binding with proper WHERE clauses. The acknowledged double-loading of labels is an acceptable trade-off for maintaining API error strings, as noted in the code comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37246 +/- ##
==========================================
+ Coverage 65.95% 65.96% +0.01%
==========================================
Files 2331 2332 +1
Lines 185505 185556 +51
Branches 7809 7809
==========================================
+ Hits 122346 122405 +59
+ Misses 51971 51963 -8
Partials 11188 11188
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Bunch of very minor stuff, plus a few questions. Code changes look reasonabe given circumstances; all Q's are around tests/test plan.
Co-authored-by: Ian Littman <iansltx@gmail.com>
2 participants
Resolves #37104
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
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.