Skip to content

[v14] Correctly filter out system roles#40762

Merged
gzdunek merged 1 commit intobranch/v14from
gzdunek/backport-40545/v14
Apr 22, 2024
Merged

[v14] Correctly filter out system roles#40762
gzdunek merged 1 commit intobranch/v14from
gzdunek/backport-40545/v14

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Apr 22, 2024

Backport #40545 to branch/v15.

Manual backport due to conflicts in types.pb.go.

* Correctly filter out system roles

* Move `SkipSystemRoles` check above `SearchKeywords` check

* Add tests

(cherry picked from commit 22a3389)
@gzdunek gzdunek added the no-changelog Indicates that a PR does not require a changelog entry label Apr 22, 2024
@gzdunek gzdunek enabled auto-merge April 22, 2024 12:02
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from Tener April 22, 2024 13:14
@gzdunek gzdunek added this pull request to the merge queue Apr 22, 2024
Merged via the queue into branch/v14 with commit a1d50be Apr 22, 2024
@gzdunek gzdunek deleted the gzdunek/backport-40545/v14 branch April 22, 2024 13:33
Comment thread api/types/role.go
// 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

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

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

Choose a reason for hiding this comment

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

Thank you kindly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants