From f291ffae11f0c3c4cefd02d98477713ed4d81968 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Mon, 5 Jan 2026 10:02:53 -0600 Subject: [PATCH] Enforce "user must be able to write to the team they're editing" in set-aside data store method (#37844) ## Testing - [x] Added/updated automated tests --- server/datastore/mysql/labels.go | 50 +++++++++++++++------------ server/datastore/mysql/labels_test.go | 34 ++++++++++++------ 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index 24841dfe5d..16647d4084 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -26,6 +26,33 @@ func (ds *Datastore) SetAsideLabels(ctx context.Context, notOnTeamID *uint, name return nil } + // Helper function to check if user has a write role on a specific team + hasWriteRoleOnTeam := func(teamID uint) bool { + for _, team := range user.Teams { + if team.ID == teamID && + (team.Role == fleet.RoleAdmin || + team.Role == fleet.RoleMaintainer || + team.Role == fleet.RoleGitOps) { + return true + } + } + return false + } + + // Helper function to check if user has a global write role (admin, maintainer, or gitops) + hasGlobalWriteRole := func() bool { + if user.GlobalRole == nil { + return false + } + return *user.GlobalRole == fleet.RoleAdmin || + *user.GlobalRole == fleet.RoleMaintainer || + *user.GlobalRole == fleet.RoleGitOps + } + + if !hasGlobalWriteRole() && (notOnTeamID == nil || !hasWriteRoleOnTeam(*notOnTeamID)) { + return ctxerr.New(ctx, "you cannot edit labels on the specified team") + } + type existingLabel struct { ID uint `db:"id"` AuthorID *uint `db:"author_id"` @@ -50,16 +77,6 @@ func (ds *Datastore) SetAsideLabels(ctx context.Context, notOnTeamID *uint, name return errCannotSetAside } - // Helper function to check if user has a global write role (admin, maintainer, or gitops) - hasGlobalWriteRole := func() bool { - if user.GlobalRole == nil { - return false - } - return *user.GlobalRole == fleet.RoleAdmin || - *user.GlobalRole == fleet.RoleMaintainer || - *user.GlobalRole == fleet.RoleGitOps - } - // Helper function to check if user has a write role on any team hasWriteRoleAnywhere := func() bool { for _, team := range user.Teams { @@ -72,19 +89,6 @@ func (ds *Datastore) SetAsideLabels(ctx context.Context, notOnTeamID *uint, name return false } - // Helper function to check if user has a write role on a specific team - hasWriteRoleOnTeam := func(teamID uint) bool { - for _, team := range user.Teams { - if team.ID == teamID && - (team.Role == fleet.RoleAdmin || - team.Role == fleet.RoleMaintainer || - team.Role == fleet.RoleGitOps) { - return true - } - } - return false - } - for _, label := range labels { if label.TeamID == nil { // Global label if notOnTeamID == nil { // Disallow moving aside since the label is on the same team diff --git a/server/datastore/mysql/labels_test.go b/server/datastore/mysql/labels_test.go index 1bc57630b5..fe608134ac 100644 --- a/server/datastore/mysql/labels_test.go +++ b/server/datastore/mysql/labels_test.go @@ -3077,7 +3077,11 @@ func testSetAsideLabels(t *testing.T, ds *Datastore) { globalAdmin := newUser(fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}) team1Admin := newUser(fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleAdmin}}}) team1Maintainer := newUser(fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleMaintainer}}}) - multiTeamUser := newUser(fleet.User{Teams: []fleet.UserTeam{ + multiTeamAdmin := newUser(fleet.User{Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleAdmin}, + {Team: fleet.Team{ID: team2.ID}, Role: fleet.RoleAdmin}, + }}) + multiTeamMaintainer := newUser(fleet.User{Teams: []fleet.UserTeam{ {Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleMaintainer}, {Team: fleet.Team{ID: team2.ID}, Role: fleet.RoleMaintainer}, }}) @@ -3135,24 +3139,34 @@ func testSetAsideLabels(t *testing.T, ds *Datastore) { expectError: true, }, { - name: "team admin can set aside their own team's labels", + name: "team admin can't set aside their own team's labels if they can't edit the not-on team", labels: []labelSpec{{name: "team1-setaside-1", teamID: &team1.ID, authorID: nil}}, notOnTeamID: &team2.ID, user: team1Admin, + expectError: true, + }, + { + name: "team admin can set aside their own team's labels if they can also edit the not-on team", + labels: []labelSpec{{name: "team1-setaside-admin", teamID: &team1.ID, authorID: nil}}, + notOnTeamID: &team2.ID, + user: multiTeamAdmin, expectError: false, }, { - name: "team maintainer can set aside their own team's labels", - labels: []labelSpec{{name: "team1-setaside-2", teamID: &team1.ID, authorID: nil}}, + name: "team maintainer can set aside their own team's labels if they can also edit the not-on team", + labels: []labelSpec{{name: "team1-setaside-maintain", teamID: &team1.ID, authorID: nil}}, notOnTeamID: &team2.ID, - user: team1Maintainer, + user: multiTeamMaintainer, expectError: false, }, { - name: "team gitops can set aside their own team's labels", - labels: []labelSpec{{name: "team1-setaside-3", teamID: &team1.ID, authorID: nil}}, + name: "team gitops can set aside their own team's labels if they can also edit the not-on team", + labels: []labelSpec{{name: "team1-setaside-gitops", teamID: &team1.ID, authorID: nil}}, notOnTeamID: &team2.ID, - user: newUser(fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleGitOps}}}), + user: newUser(fleet.User{Teams: []fleet.UserTeam{ + {Team: fleet.Team{ID: team1.ID}, Role: fleet.RoleGitOps}, + {Team: fleet.Team{ID: team2.ID}, Role: fleet.RoleGitOps}, + }}), expectError: false, }, { @@ -3228,11 +3242,11 @@ func testSetAsideLabels(t *testing.T, ds *Datastore) { { name: "multiple labels can be set aside at once", labels: []labelSpec{ - {name: "multi-setaside-1", teamID: nil, authorID: &multiTeamUser.ID}, + {name: "multi-setaside-1", teamID: nil, authorID: &multiTeamMaintainer.ID}, {name: "multi-setaside-2", teamID: &team2.ID, authorID: nil}, }, notOnTeamID: &team1.ID, - user: multiTeamUser, + user: multiTeamMaintainer, expectError: false, }, }