API + auth + UI changes for team labels by iansltx · Pull Request #37208 · fleetdm/fleet

iansltx · 2025-12-12T20:38:57Z

Covers #36760, #36758.

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/ or ee/fleetd-chrome/changes.
See Changes files for more information.
Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)

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

Turns out, we have a lot to unwind here so this doesn't compile yet

codecov · 2025-12-12T20:48:48Z

Codecov Report

❌ Patch coverage is 63.72951% with 177 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.87%. Comparing base (9adfe73) to head (f939b1b).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/labels.go	52.27%	91 Missing and 14 partials ⚠️
server/service/labels.go	58.51%	27 Missing and 12 partials ⚠️
frontend/services/entities/labels.ts	56.25%	7 Missing ⚠️
...ManageLabelsPage/LabelsTable/LabelsTableConfig.tsx	40.00%	6 Missing ⚠️
...d/fleetctl/fleetctl/testing_utils/testing_utils.go	72.72%	1 Missing and 2 partials ⚠️
server/service/metrics_labels.go	0.00%	3 Missing ⚠️
...ntend/pages/labels/EditLabelPage/EditLabelPage.tsx	66.66%	2 Missing ⚠️
...s/components/DynamicLabelForm/DynamicLabelForm.tsx	33.33%	1 Missing and 1 partial ⚠️
server/fleet/teams.go	50.00%	1 Missing and 1 partial ⚠️
server/service/global_policies.go	88.88%	1 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #37208      +/-   ##
==========================================
- Coverage   65.88%   65.87%   -0.02%     
==========================================
  Files        2360     2361       +1     
  Lines      187285   187383      +98     
  Branches     8006     8017      +11     
==========================================
+ Hits       123402   123441      +39     
- Misses      52605    52645      +40     
- Partials    11278    11297      +19

Flag	Coverage Δ
backend	`67.69% <63.25%> (-0.06%)`	⬇️
frontend	`54.40% <67.24%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Zed + Opus 4.5. Prompt below: Try to run `make test` and fix compile errors where the interface no longer matches the implementation until the compile completes. Don't change the associated interfaces, and use commit 5ad3c18 as a guide for the sort of changes that need to be made.

Courtesy Junie. Prompt: Using PlatformField.tsx as a pattern, give me a text field component (not an input, not editable) for "Team name". Include this in both ManualLabelForm and DynamicLabelForm, showing the component only if a new teamName prop (nullable string, required) is truthy. Pass this property all the way from LabelForm. There, add team_name to ILabel as an optional nullable string, and pass that to the DynamicLabelForm and ManualLabelForm components. TODO: A bunch of manual fixes, but figured I'd split those into a commit to show what's mine/what the robot did.

…scription, include teams reference

Backend isn't built here yet

… controls on label edit page and manage labels action menu

Used Zed + Opus 4.5 with the prompt: I'm getting test failures on frontend tests (`make test-js`) because I'm pretty sure real server requests are leaking through HTTP mocks. Fix the issues. It made some other changes that weren't relevant for fixing the problem, so I reverted those prior to committing.

iansltx · 2025-12-24T03:28:13Z

@lucasmrod Going to stack additional tests in another PR since this one's massive. Added TODOs on where I believe we want more test coverage. Adding that checklist to the parent issue momentarily.

lucasmrod

Overall LGTM!

Publishing review while I review test code.

lucasmrod · 2025-12-21T16:54:14Z

+  action == create
+}
+
+# Team admins, maintainers, and gitops can create global labels


(Not something to fix now) At some point in the future we need to revisit this. Team folks should not be able to configure queries that run on ALL hosts.

I think we have visibility checks elsewhere to avoid this, maybe?

lucasmrod · 2025-12-21T17:20:33Z

 		return nil
 	}

-	// Get the user from the context.


Fail early if !ok || user == nil || user.User == nil?

The previous code let user-less viewers through, so I assumed we needed to keep that behavior. But looking at code paths again, it doesn't look like we ever actually call this somewhere we don't have a ViewerContext injected, so fair enough.

lucasmrod · 2025-12-29T14:32:01Z

+		if hasWriteRoleAnywhere() && label.AuthorID != nil && *label.AuthorID == user.ID {
+			continue
+		}


Why check authorship of a team label?

For consistency, I suppose. But I'm fine with nuking this, since label authorship editability is a holdover from shimming team-ish functionality into global labels.

lucasmrod · 2025-12-29T14:51:43Z

+				rows, err := tx.QueryContext(ctx, hostTeamCheckSql, args...)
+				if err != nil {
+					return ctxerr.Wrap(ctx, err, "execute host team membership check query")
+				}
+
+				rows.Next()
+				var hostCountOnWrongTeam int
+				if err := rows.Scan(&hostCountOnWrongTeam); err != nil {
+					return ctxerr.Wrap(ctx, err, "check host team membership")
+				}
+				if err := rows.Err(); err != nil {
+					return ctxerr.Wrap(ctx, err, "check host team membership")
+				}
+				if err := rows.Close(); err != nil { //nolint:sqlclosecheck
+					return ctxerr.Wrap(ctx, err, "close result set for host team membership")
+				}


Suggested change

rows, err := tx.QueryContext(ctx, hostTeamCheckSql, args...)

if err != nil {

return ctxerr.Wrap(ctx, err, "execute host team membership check query")

}

rows.Next()

var hostCountOnWrongTeam int

if err := rows.Scan(&hostCountOnWrongTeam); err != nil {

return ctxerr.Wrap(ctx, err, "check host team membership")

}

if err := rows.Err(); err != nil {

return ctxerr.Wrap(ctx, err, "check host team membership")

}

if err := rows.Close(); err != nil { //nolint:sqlclosecheck

return ctxerr.Wrap(ctx, err, "close result set for host team membership")

}

var hostCountOnWrongTeam int

if err := tx.QueryRowxContext(ctx, hostTeamCheckSql, args...).Scan(&hostCountOnWrongTeam); err != nil {

return ctxerr.Wrap(ctx, err, "execute host team membership check query")

}

Thanks for calling this out. Somehow missed that QueryRowxContext was a thing.

lucasmrod · 2025-12-29T15:06:08Z

+				SELECT l.*,
+					(SELECT COUNT(1)
+					 FROM label_membership lm
+					     JOIN hosts h ON (lm.host_id = h.id) WHERE label_id = l.id AND %s


Nit:

Suggested change

JOIN hosts h ON (lm.host_id = h.id) WHERE label_id = l.id AND %s

JOIN hosts h ON (lm.host_id = h.id) WHERE lm.label_id = l.id AND %s

lucasmrod · 2025-12-29T16:34:46Z

 }

-func verifyLabelsToAssociate(ctx context.Context, ds fleet.Datastore, entityTeamID *uint, labelNames []string) error {
+func verifyLabelsToAssociate(ctx context.Context, ds fleet.Datastore, entityTeamID *uint, labelNames []string, user *fleet.User) error {


Let's return an error early if user == nil.

Fair enough. Will see if tests break on this, given that there are a fair number of paths that land here and ViewerContext doesn't guarantee a user.

lucasmrod · 2025-12-29T16:43:07Z

See TODOs (linking for referencing in future PRs)

RachelElysia · 2025-12-29T18:33:10Z

+    } catch (error) {
+      renderFlash(
+        "error",
+        (error as { status: number }).status === 409


I'm wondering if this 409 check might now or in the future return false negatives?

I think we're trying to use a better error handling pattern such as:

const duplicateEntryReason = getErrorReason(err, { reasonIncludes: "Duplicate entry", });

Status code should be fully reliable here, and tbh is preferable to a substring match on error message contents. For a generic 400/422, given that we don't hand back error codes, I get why we're doing substring matches, but this kinda goes back to what I mentioned in ~August of last year about having error messages that we don't have to substring to decipher.

RachelElysia · 2025-12-29T18:34:35Z

+      (isTeamAdmin(currentUser, label.team_id) ||
+        isTeamMaintainer(currentUser, label.team_id)))
+  );
+};


RachelElysia · 2025-12-29T18:40:16Z

      <LabelForm
        defaultName={defaultName}
        defaultDescription={defaultDescription}
+        teamName={teamName}


Just confirming, this feature is not supporting adding teams to labels via the UI, only via gitops, but can view that it's a team label in the UI, correct? @iansltx

That's correct. Editing team labels is in scope, creating them is not. Team is also an immutable property, and will remain immutable when we add the ability to add team labels via the UI, at least for now, since there are so many things that have to be recalculated if moving a label from one team to another (making a team specific label global is less painful).

RachelElysia · 2025-12-29T18:43:43Z

              readOnly={isEditing}
              onLoad={onLoad}
              wrapperClassName={`${baseClass}__text-editor-wrapper form-field`}
-              helpText={isEditing ? IMMUTABLE_QUERY_HELP_TEXT : ""}


I'm not quite sure this was removed on purpose? This branch has the query immutable, but it looks like you can change it with no help text saying otherwise that it's read only.

Screen.Recording.2025-12-29.at.1.42.12.PM.mov

Nm I see it's up top.

Maybe quick iteration to make this look disabled, also it's missing the copy button. Don't want to block this PR for those improvements cc: design @marko-lisica

Label query editability bug is preexisting; just checked Dogfood. Definitely missed that when clicking around when testing this though.

Re: help text, the help text is right below the description, which matches this Figma.

#34124 actually covers the missing disabled state on the query editor.

RachelElysia · 2025-12-29T18:51:07Z

+        .slice(0, -1)
+        .join(", ")} and ${immutableFields.pop()} ${SUFFIX}`;
+};
+


Need fix: Somehow this function is missing platforms when I tested this:

probably the .slice

RachelElysia

Approving so we won't need to re-review from BE, but will need an immediate FE followup PR to address this copy bug:
https://github.com/fleetdm/fleet/pull/37208/files#r2651557303

iansltx · 2025-12-30T03:28:40Z

Building follow-up PRs (split into BE/FE) to address feedback. Merging this as tests currently pass, and per earlier discussion today.

Once additional feedback is in I'll create a cherry-pick into 4.79RC with everything.

iansltx added 5 commits December 11, 2025 13:00

Start adding TODOs for API-related auth changes

1a7efa7

WIP add team name to label GET response

a1f32d3

Turns out, we have a lot to unwind here so this doesn't compile yet

Add a TODO

fa4959e

Finish label team name updates

41f3ddb

Update mocks

4dce79f

iansltx temporarily deployed to Docker Hub December 12, 2025 20:39 — with GitHub Actions Inactive

iansltx added 3 commits December 12, 2025 14:49

Fix some interface conformance errors

5ad3c18

Fix labels query to use table alias

ac3b885

iansltx temporarily deployed to Docker Hub December 12, 2025 21:05 — with GitHub Actions Inactive

Fix another test

a4d6200

iansltx temporarily deployed to Docker Hub December 12, 2025 21:45 — with GitHub Actions Inactive

iansltx added 3 commits December 12, 2025 16:26

...actually some stuff I did was in the previous commit...anyway...

1118fc5

Fix tests

96fa479

iansltx had a problem deploying to Docker Hub December 15, 2025 16:10 — with GitHub Actions Failure

Move immutability help text from each form field to combined below de…

2d775f9

…scription, include teams reference

iansltx had a problem deploying to Docker Hub December 15, 2025 17:39 — with GitHub Actions Failure

Scope entity associated label list API calls by team ID from UI

c7d0e7a

Backend isn't built here yet

iansltx had a problem deploying to Docker Hub December 15, 2025 23:33 — with GitHub Actions Failure

Show cleaner error message for duplicate label name, implement access…

c69dbe6

… controls on label edit page and manage labels action menu

iansltx had a problem deploying to Docker Hub December 16, 2025 05:22 — with GitHub Actions Failure

Merge branch 'main' into 36758-team-labels-api

a7d2960

iansltx had a problem deploying to Docker Hub December 16, 2025 05:52 — with GitHub Actions Failure

Fix lint + test

6a5246f

iansltx had a problem deploying to Docker Hub December 16, 2025 06:02 — with GitHub Actions Failure

Add team name field component that I should've committed a bit ago

1cff9b8

iansltx temporarily deployed to Docker Hub December 16, 2025 06:06 — with GitHub Actions Inactive

iansltx had a problem deploying to Docker Hub December 24, 2025 03:26 — with GitHub Actions Error

Merge branch 'main' into 36758-team-labels-api

f939b1b

iansltx marked this pull request as ready for review December 24, 2025 03:27

iansltx requested review from a team as code owners December 24, 2025 03:27

iansltx temporarily deployed to Docker Hub December 24, 2025 03:27 — with GitHub Actions Inactive

iansltx changed the title ~~[WIP] Team labels work~~ API + UI changes for team labels Dec 24, 2025

iansltx changed the title ~~API + UI changes for team labels~~ API + auth + UI changes for team labels Dec 24, 2025

iansltx mentioned this pull request Dec 24, 2025

Team admin, maintainer, and GitOps users can create labels via GitOps (YAML) #33760

Closed

32 tasks

Copilot AI mentioned this pull request Dec 24, 2025

Add team label tests to replace TODOs from PR #37208 #37669

Closed

2 tasks

iansltx marked this pull request as draft December 26, 2025 21:01

lucasmrod reviewed Dec 29, 2025

View reviewed changes

RachelElysia self-assigned this Dec 29, 2025

lucasmrod mentioned this pull request Dec 29, 2025

GitOps changes for team labels #36759

Closed

lucasmrod marked this pull request as ready for review December 29, 2025 17:22

lucasmrod approved these changes Dec 29, 2025

View reviewed changes

RachelElysia reviewed Dec 29, 2025

View reviewed changes

RachelElysia approved these changes Dec 29, 2025

View reviewed changes

iansltx merged commit 8e4e89f into main Dec 30, 2025
52 checks passed

iansltx deleted the 36758-team-labels-api branch December 30, 2025 03:28

iansltx added a commit that referenced this pull request Dec 30, 2025

Address backend review feedback from #37208

fdbe5bf

Copilot AI mentioned this pull request Dec 30, 2025

Remove redundant label validation in BatchValidateLabels #37746

Closed

2 tasks

iansltx mentioned this pull request Jan 1, 2026

Cherry-Pick: API, generate-gitops, FE changes for team labels #37790

Merged

	JOIN hosts h ON (lm.host_id = h.id) WHERE label_id = l.id AND %s
	JOIN hosts h ON (lm.host_id = h.id) WHERE lm.label_id = l.id AND %s

Uh oh!

Conversation

iansltx commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Testing

Uh oh!

codecov Bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

iansltx commented Dec 24, 2025

Uh oh!

lucasmrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RachelElysia left a comment

Choose a reason for hiding this comment

Uh oh!

iansltx commented Dec 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iansltx commented Dec 12, 2025 •

edited

Loading

codecov Bot commented Dec 12, 2025 •

edited

Loading