[Connect] Conditionally render Access Request navigation menu#1273
[Connect] Conditionally render Access Request navigation menu#1273
Conversation
The Access Requests navigation menu should now only show when a root cluster's auth server has Advanced Workflows feature enabled.
| async logout(clusterUri: string) { | ||
| // TODO(gzdunek): logout and removeCluster should be combined into a single acton in tshd | ||
| await this.client.logout(clusterUri); | ||
| await this.syncClusterInfo(clusterUri); |
There was a problem hiding this comment.
Why would we need to try and syncClusterInfo after logging out? I removed it because now that GetCluster has a addMetadataToRetryableError, logging out would fail here during the sync. "no SSH auth methods found. are you logged in?". we just logged out the line above, of course we aren't logged in! I think this line might have just been an error from the start (git blame says back in febuary) and it wasn't never noticed since it was essentially ignoring the error.
There was a problem hiding this comment.
Just for posterity, I'm linking my comment from another PR gravitational/teleport#17497 (comment) which explains that this is probably a remnant of times when logging out and removing a cluster from the list were two separate actions.
|
I forgot to request review from you both @ravicious and @gzdunek yesterday when I posted the backend portion of this. This might have given a better context of my backend change. Regardless, I appreciate the great detail you left there |
| const selectorRef = useRef<HTMLButtonElement>(); | ||
|
|
||
| if (!activeRootCluster) { | ||
| if ( |
There was a problem hiding this comment.
Maybe something like this? It seems easier to understand
const shouldShowMenu = !!activeRootCluster?.features.advancedAccessWorkflows;
if (!shouldShowMenu) {
return null;
}
| async logout(clusterUri: string) { | ||
| // TODO(gzdunek): logout and removeCluster should be combined into a single acton in tshd | ||
| await this.client.logout(clusterUri); | ||
| await this.syncClusterInfo(clusterUri); |
There was a problem hiding this comment.
Just for posterity, I'm linking my comment from another PR gravitational/teleport#17497 (comment) which explains that this is probably a remnant of times when logging out and removing a cluster from the list were two separate actions.
| if (!activeRootCluster) { | ||
| if ( | ||
| !activeRootCluster || | ||
| !activeRootCluster.features.advancedAccessWorkflows |
There was a problem hiding this comment.
I'd assume that when we get a response from endpoint that doesn't actually populate features, such as ListRootClusters, then features is going to be undefined. But I'm surprised to see it's actually the whole struct with fields simply set to false. Interesting, I didn't know that.
There was a problem hiding this comment.
I think this is bad design. I've updated the backend to only return the features on the relevant rpcs (such as GetCluster). Although I think in the future we may want to rename ListRootClusters to something like ListRootClustersInProfile since that more clearly describes what it does
There was a problem hiding this comment.
Although I think in the future we may want to rename
ListRootClustersto something likeListRootClustersInProfilesince that more clearly describes what it does
Another idea for the future would be to utilize the render view pattern that Alan mentioned. But yeah, we're free to completely change the RPCs around getting root cluster details, there's nothing holding us back. They work the way they work right now because we've never had to do more than just read stuff from the disk.
| const selectorRef = useRef<HTMLButtonElement>(); | ||
|
|
||
| if (!activeRootCluster) { | ||
| if ( |
There was a problem hiding this comment.
Would it make sense to put the access requests menu to the left of profile selector? Otherwise the profile selector switches position when switching between a cluster that has access requests and one that doesn't.
The Access Requests navigation menu should now only show when a root cluster's auth server has Advanced Workflows feature enabled.
3 participants
Relies on gravitational/teleport#17497
Right now, this menu will show on any cluster. This could lead to bad UX if a user thinks they have access to creating requests just to receive a permission error when they try to submit a new request. This change will not display the menu if the active root cluster does not have the access request feature enabled. Only two files were changed,
NavigationMenu, andclustersServicethe rest are protobufs from the linked PR.Example of enterprise/non-enterprise TopMenus
https://user-images.githubusercontent.com/5201977/196263701-fff57268-514a-4e13-a4a8-b7de41860a69.mp4