Display permission banner on Enroll Resource page

[avatus]

This adds an info banner to the Enroll Resource page if the user has no permissions to add any resource kind. Unlike other banners we've added recently to this, we decided not to have the "granular" list of permissions needed due to the length of possible permission states.

Contributes to https://github.com/gravitational/teleport.e/issues/4978

[avatus]

I snuck in a small change to features to always show the enroll integration button in nav based on a currently open similar change in e

[ravicious]

Suggested change

	test('does not dislpay erorr banner if user has permissions to add', async () => {
	test('does not display error banner if user has permissions to add', async () => {

[ravicious]

Nit, but it should be enough to do this, no?

Suggested change

	const userContext = ctx.storeUser.state;
	const { acl, authType } = userContext;
	const { acl, authType } = ctx.storeUser.state;

[ravicious]

Don't be sorry. 😏

Suggested change

// sorry this looks gnarly.

[ravicious]

Iterating through what kind of resources array we're talking about here? If it's just some static array in the code, wouldn't it be better to iterate through that so that we're sure that this check here is up to date? Otherwise adding a new kind of resource will require updating this check as well (which is not going to be easy to remember about).

[avatus]

thats a great point actually. i think i will switch over to the iteration method because it'll catch any resource type we add in the future. thanks for catching that

[avatus]

The update avoids my giant logic bubble as well, so a much greater change

[ravicious]

You might want to pass a function to useState to avoid calling addHasAccessField and filterResources on each render. It likely won't have a big impact on performance, but at least we won't be calculating something on each render and then discarding the result. ;)

[ravicious]

I noticed that defaultResources are calculated only once. Something like this would work even better and avoid some of the logic living in useEffect.

diff --git a/web/packages/teleport/src/Discover/SelectResource/SelectResource.tsx b/web/packages/teleport/src/Discover/SelectResource/SelectResource.tsx
index d8fa09e0645..a289dfba151 100644
--- a/web/packages/teleport/src/Discover/SelectResource/SelectResource.tsx
+++ b/web/packages/teleport/src/Discover/SelectResource/SelectResource.tsx
@@ -16,7 +16,12 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import { useEffect, useState, type ComponentPropsWithoutRef } from 'react';
+import {
+  useEffect,
+  useMemo,
+  useState,
+  type ComponentPropsWithoutRef,
+} from 'react';
 import { useHistory, useLocation } from 'react-router';
 import styled from 'styled-components';
 
@@ -64,6 +69,10 @@ type UrlLocationState = {
   searchKeywords: string;
 };
 
+const RESOURCES = !cfg.isEnterprise
+  ? BASE_RESOURCES
+  : [...BASE_RESOURCES, ...SAML_APPLICATIONS];
+
 export function SelectResource({ onSelect }: SelectResourceProps) {
   const ctx = useTeleport();
   const location = useLocation<UrlLocationState>();
@@ -71,20 +80,24 @@ export function SelectResource({ onSelect }: SelectResourceProps) {
   const { preferences } = useUser();
 
   const [search, setSearch] = useState('');
-  const RESOURCES = !cfg.isEnterprise
-    ? BASE_RESOURCES
-    : [...BASE_RESOURCES, ...SAML_APPLICATIONS];
   const { acl, authType } = ctx.storeUser.state;
   const platform = getPlatform();
-  const [resources, setResources] = useState<ResourceSpec[]>(
-    addHasAccessField(acl, filterResources(platform, authType, RESOURCES))
+  const defaultResources: ResourceSpec[] = useMemo(
+    () =>
+      sortResources(
+        // Apply access check to each resource.
+        addHasAccessField(acl, filterResources(platform, authType, RESOURCES)),
+        preferences,
+        storageService.getOnboardDiscover()
+      ),
+    [acl, authType, platform, preferences]
   );
+  const [resources, setResources] = useState(defaultResources);
 
   // a user must be able to create tokens AND have access to create at least one
   // type of resource in order to be considered eligible to "add resources"
   const canAddResources = acl.tokens.create && resources.some(r => r.hasAccess);
 
-  const [defaultResources, setDefaultResources] = useState<ResourceSpec[]>([]);
   const [showApp, setShowApp] = useState(false);
 
   function onSearch(s: string, customList?: ResourceSpec[]) {
@@ -104,15 +117,6 @@ export function SelectResource({ onSelect }: SelectResourceProps) {
   }
 
   useEffect(() => {
-    // Apply access check to each resource.
-    const onboardDiscover = storageService.getOnboardDiscover();
-    const sortedResources = sortResources(
-      resources,
-      preferences,
-      onboardDiscover
-    );
-    setDefaultResources(sortedResources);
-
     // A user can come to this screen by clicking on
     // a `add <specific-resource-type>` button.
     // We sort the list by the specified resource type,
@@ -128,7 +132,7 @@ export function SelectResource({ onSelect }: SelectResourceProps) {
     ) {
       const sortedResourcesByKind = sortResourcesByKind(
         resourceKindSpecifiedByUrlLoc,
-        sortedResources
+        defaultResources
       );
       onSearch(resourceKindSpecifiedByUrlLoc, sortedResourcesByKind);
       return;
@@ -136,11 +140,11 @@ export function SelectResource({ onSelect }: SelectResourceProps) {
 
     const searchKeywordSpecifiedByUrlLoc = location.state?.searchKeywords;
     if (searchKeywordSpecifiedByUrlLoc) {
-      onSearch(searchKeywordSpecifiedByUrlLoc, sortedResources);
+      onSearch(searchKeywordSpecifiedByUrlLoc, defaultResources);
       return;
     }
 
-    setResources(sortedResources);
+    setResources(defaultResources);
     // Processing of the lists should only happen once on init.
     // User perms remain static and URL loc state does not change.
     // eslint-disable-next-line react-hooks/exhaustive-deps

[ravicious]

This also made me wonder: should canAddResources be based on defaultResources instead of resources? The way I understand resources work is that you can filter down the resources on the Discover list by name and kind. If a user filters down the list to only resources that they cannot add, but defaultResources contains some that they can add, should we display the banner?

[avatus]

we ran into an issue before (elsewhere) with config being outside some sort of function call. it would mean that cfg would be calculated on page load and not ever receive the update to it once webconfig.js gets returned from the network call. so i think ill make it a function and pass the cfg.isEnterprise value to it instead of calculating all outside.

also, canAddResources should never change based on the filter so we should base it on default resources and thats it.

lastly, since canAddResources shouldnt really ever change since its based on defaults + acl, i could just memoize that as well. but i decided not too since it really isnt that expensive of a calculation

[ravicious]

we ran into an issue before (elsewhere) with config being outside some sort of function call. it would mean that cfg would be calculated on page load and not ever receive the update to it once webconfig.js gets returned from the network call. so i think ill make it a function and pass the cfg.isEnterprise value to it instead of calculating all outside.

This would be a good argument for deprecating direct access to cfg and replacing it with some kind of a function perhaps.

[kimlisa]

we could probably use the same logic to integrations picker... only show banner in the strictest case (missing all perms)

[public-teleport-github-review-bot]

@avatus See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Create PR