Skip to content

[v14] Correctly filter out system roles #40762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gzdunek merged 1 commit into from
Apr 22, 2024
Merged

[v14] Correctly filter out system roles #40762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2581,6 +2581,8 @@ message PluginDataUpdateParams {
message RoleFilter {
// SearchKeywords is a list of search keywords to match against resource field values.
repeated string SearchKeywords = 1 [(gogoproto.jsontag) = "search_keywords,omitempty"];
// SkipSystemRoles filters out teleport system roles from the results.
bool SkipSystemRoles = 2 [(gogoproto.jsontag) = "skip_system_roles,omitempty"];
}

// RoleV6 represents role resource specification
Expand Down
4 changes: 4 additions & 0 deletions api/types/role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ const (

// Match checks if the given role matches this filter.
func (f *RoleFilter) Match(role *RoleV6) bool {
if f.SkipSystemRoles {
return !IsSystemResource(role)
Copy link
Copy Markdown

@patrikjuvonen patrikjuvonen Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there @gzdunek, I think this line might have introduced a bug (at least on v14.3.16).

Right now if you search for roles in the web UI (at least within the Edit User modal), it will never reach the search keywords part below, thus search will never return filtered results by keyword.

We found this regression on one of our production instances while trying to add a new role to a user, search is always returning the same list regardless of the keyword. We have some hundreds of roles so keyword search is very useful to us. Meanwhile we've resorted to the CLI for now to manage user roles.

I was also looking at #40464 if the bug is related to the new pagination, but this line caught my eye because of the early return.

cc #40545

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an obvious bug 🤦
I shouldn't have changed the checks order.

Sorry about that, I will send a PR with a fix soon.
Thanks for reaching out.

Copy link
Copy Markdown

@patrikjuvonen patrikjuvonen Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you kindly!

}

if len(f.SearchKeywords) != 0 {
if !role.MatchSearch(f.SearchKeywords) {
return false
Expand Down
62 changes: 62 additions & 0 deletions api/types/role_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,3 +437,65 @@ func TestUnmarshallCreateHostUserModeYAML(t *testing.T) {
require.Equal(t, tc.expected, got)
}
}

func TestRoleFilterMatch(t *testing.T) {
regularRole := RoleV6{
Metadata: Metadata{
Name: "request-approver",
},
}
systemRole := RoleV6{
Metadata: Metadata{
Name: "bot",
Labels: map[string]string{
TeleportInternalResourceType: SystemResource,
},
},
}

tests := []struct {
name string
role *RoleV6
filter *RoleFilter
shouldMatch bool
}{
{
name: "empty filter should match everything",
role: &regularRole,
filter: &RoleFilter{},
shouldMatch: true,
},
{
name: "correct search keyword should match the role",
role: &regularRole,
filter: &RoleFilter{SearchKeywords: []string{"appr"}},
shouldMatch: true,
},
{
name: "incorrect search keyword shouldn't match the role",
role: &regularRole,
filter: &RoleFilter{SearchKeywords: []string{"xyz"}},
shouldMatch: false,
},
{
name: "skip system roles filter shouldn't match the system role",
role: &systemRole,
filter: &RoleFilter{SkipSystemRoles: true},
shouldMatch: false,
},
{
name: "skip system roles filter should match the regular role",
role: &regularRole,
filter: &RoleFilter{SkipSystemRoles: true},
shouldMatch: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.role.CheckAndSetDefaults()
require.NoError(t, err)
require.Equal(t, tt.shouldMatch, tt.filter.Match(tt.role))
})
}
}
Loading