Skip to content

Manually sort pinned resources and UI fixes#33470

Merged
avatus merged 2 commits intomasterfrom
avatus/pinning_fixes
Oct 13, 2023
Merged

Manually sort pinned resources and UI fixes#33470
avatus merged 2 commits intomasterfrom
avatus/pinning_fixes

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Oct 13, 2023

A couple small bug fixes found with pinned resources.

  1. pinned resources are not fetched by ascending our sort tree so they would be returned in the order they were pinned. Because of this, we need to manually sort (the same way we used to before sort indexes).
  2. A fix went in last week to properly filter SAML apps since they are not included in the resourceChecker but I forgot to add it in the matchFn for pinned resources, so I added that in here.
  3. Was incorrectly using a TextIcon component (with no icon) for the not supported or no resources messages which caused it to be slightly uncentered
    before
Screenshot 2023-10-13 at 12 11 29 PM

after

Screenshot 2023-10-13 at 12 11 35 PM

@bl-nero bl-nero self-requested a review October 13, 2023 17:51
Comment thread lib/auth/auth_with_roles.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we check this if in both cases we return false?

Comment thread lib/auth/auth_with_roles.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm gonna accept this change as a stop-gap measure, but it sounds really strange and error-prone, especially security-wise, to do it like this. If we have a function that's called CanAccess on something that is called resourceChecker, we shouldn't expect the consumers of this function to understand that in a specific case of types.SAMLIdPServiceProvider, they should call something else. I think that this should be folded into either of these in the long term. Is there something I don't understand?

Copy link
Copy Markdown
Contributor Author

@avatus avatus Oct 13, 2023

Choose a reason for hiding this comment

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

Will add a TODO to fold SAML into this once I figure out why it wasn't there in the first place. Thanks

* Manually sort pinned resource requests and update match filter

* Use Text component for not supported message
@avatus avatus force-pushed the avatus/pinning_fixes branch from cd71b8a to e5fe720 Compare October 13, 2023 18:21
@avatus avatus enabled auto-merge October 13, 2023 18:25
@avatus avatus added this pull request to the merge queue Oct 13, 2023
Merged via the queue into master with commit 5b85848 Oct 13, 2023
@avatus avatus deleted the avatus/pinning_fixes branch October 13, 2023 19:02
@public-teleport-github-review-bot
Copy link
Copy Markdown

@avatus See the table below for backport results.

Branch Result
branch/v14 Create PR

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants