Trim spaces on Fleet's names (36312)#41739
Conversation
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41739 +/- ##
========================================
Coverage 66.42% 66.42%
========================================
Files 2501 2511 +10
Lines 200169 200817 +648
Branches 9002 8896 -106
========================================
+ Hits 132958 133396 +438
- Misses 55204 55365 +161
- Partials 12007 12056 +49
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:
|
- Validate and trim fleet names in NewTeam, ModifyTeam, and ApplyTeamSpecs - Trim fleet names in gitops YAML parsing (parseName) - Disable submit button in CreateTeamModal and RenameTeamModal when name is whitespace-only - Add migration to trim existing fleet names, disambiguate conflicts, and rename whitespace-only names to "Unnamed team (<id>)"
juan-fdz-hawa
force-pushed
the
36312-team-names-can-be-all-white-space-in-api
branch
from
891acb2 to
8ece8de
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request implements team name validation by trimming leading and trailing whitespace across multiple layers. Changes include backend service modifications to trim names during team creation and modification, frontend updates to disable submit buttons for whitespace-only input, GitOps spec parsing updates to trim names, and a database migration that cleans existing team names and handles edge cases such as renaming whitespace-only names to "Unnamed team ()" and disambiguating conflicts. Comprehensive test coverage is added for new validation behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/teams.go (1)
1060-1061:⚠️ Potential issue | 🟠 MajorNormalize spec names before authorization checks.
ApplyTeamSpecsauthorizes using the untrimmedspec.Name, then trims later. A padded name can fail lookup during auth but resolve to an existing team afterward, producing incorrect authorization outcomes.💡 Suggested fix
func (svc *Service) ApplyTeamSpecs(ctx context.Context, specs []*fleet.TeamSpec, applyOpts fleet.ApplyTeamSpecOptions) ( map[string]uint, error, ) { if len(specs) == 0 { setAuthCheckedOnPreAuthErr(ctx) // Nothing to do. return map[string]uint{}, nil } + + for _, spec := range specs { + spec.Name = strings.TrimSpace(spec.Name) + if spec.Name == "" { + return nil, fleet.NewInvalidArgumentError("name", "name may not be empty") + } + } if err := svc.checkAuthorizationForTeams(ctx, specs); err != nil { return nil, err } @@ - spec.Name = strings.TrimSpace(spec.Name) - if spec.Name == "" { - return nil, fleet.NewInvalidArgumentError("name", "name may not be empty") - } if fleet.IsReservedTeamName(spec.Name) { return nil, fleet.NewInvalidArgumentError("name", fmt.Sprintf("%q is a reserved fleet name", spec.Name)) }Also applies to: 1087-1090
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/teams.go` around lines 1060 - 1061, ApplyTeamSpecs is calling svc.checkAuthorizationForTeams with spec.Name values that are trimmed later, causing authorization to run on unnormalized names; normalize each spec's Name (e.g., strings.TrimSpace on spec.Name, and any other canonicalization used elsewhere) before calling checkAuthorizationForTeams (and the second authorization call around the 1087-1090 region) so the authorization check uses the same normalized names as later processing; update the code paths where specs are passed to checkAuthorizationForTeams to use the normalized spec.Name values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/teams_test.go`:
- Around line 363-371: The dry-run assertion is too weak: keep the existing
positive check for the normalized name (require.Contains(t, result,
tc.wantName)) but also assert the padded original isn't present by adding
require.NotContains(t, result, tc.teamName) (or otherwise assert the padded
string like " Engineering " is absent); this should be added in the else
branch after calling svc.ApplyTeamSpecs to prove trimming occurred for
TeamSpec.Name in ApplyTeamSpecs.
In
`@frontend/pages/admin/TeamManagementPage/components/RenameTeamModal/RenameTeamModal.tests.tsx`:
- Around line 54-56: The test currently uses userEvent.type(nameInput, "\t\t\t")
which triggers Tab focus changes instead of inserting literal tab characters;
replace that line with a direct value update (e.g., use
fireEvent.change(nameInput, { target: { value: "\t\t\t" } }) or
userEvent.paste(nameInput, "\t\t\t")) so the input actually contains only tabs
after userEvent.clear(nameInput) and the whitespace-only rejection is tested
correctly; locate the call to userEvent.type and the nameInput variable in the
RenameTeamModal tests to apply this change.
In `@server/datastore/mysql/migrations/tables/20260315000000_TrimTeamNames.go`:
- Around line 33-37: The migration currently sets names with SET name =
CONCAT('Unnamed team (', id, ')') (and similar for trimmed names) which can
collide with existing team names and exceed varchar(255); replace this with a
two-step, collision-safe allocation: first SELECT the affected rows (where
trimExpr condition holds), compute candidate names using a deterministic,
length-bounded scheme (e.g., LEFT(CONCAT('Unnamed team (', id, '-',
LEFT(SHA1(id),8), ')'), 255) or LEFT(CONCAT(<trimmed>, ' (', id, '-',
LEFT(SHA1(id),8), ')'), 255)), ensure uniqueness by checking against existing
teams and previously generated candidates (use a temporary table or
in-transaction temp mapping keyed by id), then UPDATE teams JOIN that temp
mapping to set the new name; apply the same approach for both places that use
trimExpr and the later block (lines ~47-56) to guarantee collision-free,
length-safe names.
---
Outside diff comments:
In `@ee/server/service/teams.go`:
- Around line 1060-1061: ApplyTeamSpecs is calling
svc.checkAuthorizationForTeams with spec.Name values that are trimmed later,
causing authorization to run on unnormalized names; normalize each spec's Name
(e.g., strings.TrimSpace on spec.Name, and any other canonicalization used
elsewhere) before calling checkAuthorizationForTeams (and the second
authorization call around the 1087-1090 region) so the authorization check uses
the same normalized names as later processing; update the code paths where specs
are passed to checkAuthorizationForTeams to use the normalized spec.Name values.
🪄 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: 7487c0da-2405-4619-a1cc-6c8c70f3c814
📒 Files selected for processing (13)
changes/36312-trim-spaces-from-fleets-namesee/server/service/teams.goee/server/service/teams_test.gofrontend/pages/admin/TeamManagementPage/components/CreateTeamModal/CreateTeamModal.tests.tsxfrontend/pages/admin/TeamManagementPage/components/CreateTeamModal/CreateTeamModal.tsxfrontend/pages/admin/TeamManagementPage/components/RenameTeamModal/RenameTeamModal.tests.tsxfrontend/pages/admin/TeamManagementPage/components/RenameTeamModal/RenameTeamModal.tsxpkg/spec/gitops.gopkg/spec/gitops_test.goserver/datastore/mysql/migrations/tables/20260315000000_TrimTeamNames.goserver/datastore/mysql/migrations/tables/20260315000000_TrimTeamNames_test.goserver/datastore/mysql/schema.sqlserver/service/integration_enterprise_test.go
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Error
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
code lgtm but question about whether we can skip the migration...
| { | ||
| name: "only tabs", | ||
| teamName: ptr.String("\t\t\t"), | ||
| wantErr: "may not be empty", | ||
| }, | ||
| { | ||
| name: "only newlines", | ||
| teamName: ptr.String("\n\n\n"), | ||
| wantErr: "may not be empty", | ||
| }, | ||
| { | ||
| name: "only carriage returns", | ||
| teamName: ptr.String("\r\r\r"), | ||
| wantErr: "may not be empty", | ||
| }, | ||
| { | ||
| name: "mixed whitespace", | ||
| teamName: ptr.String(" \t\n\r "), | ||
| wantErr: "may not be empty", | ||
| }, |
There was a problem hiding this comment.
Was the migration a hard requirement? Rather than deciding how to solve this for the user (in the rare case this exists) we could just leave it alone and if they go to rename the team, they'll be forced to choose a non-whitespace-only (and non-conflicting) name.
There was a problem hiding this comment.
👍 - We would just need to remove the trimming logic from the API endpoints and narrow the scope to only reject "empty" team names (e.g., " "). To your point, this makes for a much safer changeset.
As currently implemented in this PR, team names are normalized before being persisted (e.g., ' Team 1 ' becomes 'Team 1'). Without the migration, this breaks gitops: if 'Team 1 ' is defined in GitOps, applying it would attempt to create a new team.
@MagnusHJensen @rachaelshaw any objections to reducing the scope to just rejecting empty team names?
There was a problem hiding this comment.
I'll leave that call up to Rachael
There was a problem hiding this comment.
Why would we need to drop the trimming? That seems like a valuable add. I was just suggesting we could do without the migration of existing teams.
There was a problem hiding this comment.
Why would we need to drop the trimming? That seems like a valuable add. I was just suggesting we could do without the migration of existing teams.
Suppose the user has a gitops team YAML artifact with something like:
# team_one.yml
name: ' Team One '
- User runs gitops apply
- Team name gets trimmed over here - now becomes "Team One"
ds.TeamByName(ctx, spec.Name)throws a "not found" error, which means thatsvc.createTeamFromSpecwill be called- User ends up with two teams: "Team One" and " Team One " ... not bueno
There was a problem hiding this comment.
Ahhh gotcha. Yeah that's a pickle. And even if we run the migration, what happens the next time someone runs their GitOps with a file that still has a bad team name? This is a product call but I'd say we make GitOps start failing on these bad names and force folks to do the renaming themselves. This was reported internally, so unless @MagnusHJensen knows otherwise there might not even be cases like this in the wild.
There was a problem hiding this comment.
what happens the next time someone runs their GitOps with a file that still has a bad team name?
With the migration, it should do the right thing - because the Team Name is always normalized at the API/Service layer.
There are two caveats wit the changes here:
- The team name inside the YAML will say " Team One ", but if they fetch the team via the API it will say "Team One" ... a similar behavior happens currently with the UI, even if the stored team name is " Team One ", the UI always renders it as "Team One", so hopefully this isn't too bad of a thing.
- If they do a gitops generate they will see "Team One" on the exported file (which is what we want), which might be a little confusing?
There was a problem hiding this comment.
I think those caveats are both fine. I still feel like there's edge cases though... like if your migration found two teams whose cleaned-up names would conflict, it would rename them to say "Team Name 1" and "Team Name 2", but then if you had a GitOps file with just Team Name it wouldn't conflict with anything so it would change that team to Team Name.
Also I remembered that GitOps won't create a new team just because it detects a name change, as long as the filename remains the same (otherwise you wouldn't be able to rename teams with GitOps). See
fleet/ee/server/service/teams.go
Lines 1095 to 1102 in 8ece8de
Team Name in your .yml and in the database, the next time you ran fleetctl gitops after this change, it would update the db to have Team Name (and if that conflicted with another now-trimmed name, it would error, which is good).
I'm not trying to drag this out. The migration just feels slightly riskier than is warranted by the change, especially given that there might not be anyone in the wild that this actually applies to.
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
deleted the
36312-team-names-can-be-all-white-space-in-api
branch
3 participants
Related issue: Resolves #36312
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.
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 redirectsTesting
Database migrations
Summary by CodeRabbit
Bug Fixes
Tests