Disabled kinds from filter menu based on access

[avatus]

This PR solves two recent issues that came up.
Previously (before the unified resources view), the dashboard tenants would not see any "resource" options in their navigation as each item was hidden based on access. The new unified resources tab didn't have those checks so I've added them in. Dashboard tentants will no longer see any resources tab after this change. Similarly, if a user doesn't have access to any of the types (but isn't a dashboard tenant), they also won't see the resources tab. This is a fix to a regression.

Second, this updates the filter menu in the unified view to only be visible if they have more than 1 kind available to view, and removes kinds they cannot view from the filter menu. (below is an example if I don't have access to databases)

Fixes: https://github.com/gravitational/cloud/issues/6519

[avatus]

This requires a complete review as it's a very different solution now. Previously, I would just hide the kinds if the user didn't have access. Now this disabled the use of the menu item in the filter itself.

[kimlisa]

i was just curious what it looked like with all deny rules (asteriks) and this is what i got.

i'm still able to query 🤔 . was this change intentional? unless my cluster is messed up (happens a lot 😅 ), looking at my config, i'm still able to query probably because my labels allow it so... but i thought deny rules had precedence over everything else?

as an example, for listResources and getXXX, we do a preliminary action check like below. i'm not sure unified resources does this check (i see we do invidiual resource check) but maybe i'm blind.

func (a *ServerWithRoles) GetNodes(ctx context.Context, namespace string) ([]types.Server, error) {
	if err := a.action(namespace, types.KindNode, types.VerbList); err != nil {
		return nil, trace.Wrap(err)
	}

kind: role
metadata:
  id: 1699427772659522000
  name: deny-all
  revision: f2ad4828-80df-4254-8058-f51de875fd8d
spec:
  allow:
    app_labels:  // <--- these labels is what is enabling me to still query despite my deny rules
      '*': '*'
    db_labels:
      '*': '*'
    db_names:
    - '{{internal.db_names}}'
    - '*'
    db_users:
    - '{{internal.db_users}}'
    - developer
    kubernetes_groups:
    - '{{internal.kubernetes_groups}}'
    - developer
    kubernetes_labels:
      '*': '*'
    kubernetes_resources:
    - kind: '*'
      name: '*'
      namespace: '*'
      verbs:
      - '*'
    kubernetes_users:
    - '{{internal.kubernetes_users}}'
    - dev
    logins:
    - '{{internal.logins}}'
    - ubuntu
    - debian
    node_labels:
      '*': '*'
    rules:
    - resources:
      - event
      verbs:
      - list
      - read
    - resources:
      - session
      verbs:
      - read
      - list
      where: contains(session.participants, user.metadata.name)
    windows_desktop_labels:
      '*': '*'
    windows_desktop_logins:
    - '{{internal.windows_logins}}'
    - developer
  deny:
    rules:
    - resources:
      - '*'
      verbs:
      - '*'
  options:
    cert_format: standard
    create_db_user: false
    create_desktop_user: false
    desktop_clipboard: true
    desktop_directory_sharing: true
    enhanced_recording:
    - command
    - network
    forward_agent: false
    idp:
      saml:
        enabled: true
    max_session_ttl: 8h0m0s
    pin_source_ip: false
    port_forwarding: true
    record_session:
      default: best_effort
      desktop: true
    ssh_file_copy: true
version: v7

[avatus]

i was just curious what it looked like with all deny rules (asteriks) and this is what i got.

This seems unintentional. I've updated this PR to include the actionVerbs check in auth_with_roles.ListUnifiedResources.
Could you poke around again and see if it works as expected now?

[kimlisa]

you should double check for the others, but i know nodes, only does list check in the back

[avatus]

looks like only nodes does list and everyting else is both. thank you

[kimlisa]

i would model these verbList with previous implementations because we don't want to deny access to something we gave access for previously (example is nodes, for some reason, it only checks for rbac list, but now we require read here as well, so access may break for some)

[kimlisa]

when did we release this feature again? i'm a little worried about the implication of this bug for previous v14 versions. not sure what the right protocol is, or perhaps i'm just over-thinking

[kimlisa]

would there be any performance issues because it seems we are checking rbac repeatedly for the same resource kind? (can we do it once per resource kind?) i would get a backend person to review this just in case 😅

[avatus]

cc: @rosstimothy

[avatus]

You know, I don't even think this is a good solution either. This causes my backend to get 20+ INFO RBAC Access not allowed to list type of messages. (edit: i was able to suppress this by using quietAction instead)

What I'm curious about is why this isn't checked in CanAccess as it seems like it should be there. Perhaps I'm missing context, but I think that the deny rule should be caught in CanAccess (edit: after talking with Nic, we should be checking for verbs because CanAccess is more about access to backend state rather than listing.)

As far as performance goes, running the benchmark with this added took it from from ~2.8s to ~3.2s which is still in line with BenchmarkListNodes

[rosstimothy]

Yeah I think we would benefit from building a local cache of which resources the user is allowed to read/list and using that later on instead of checking for every resource.

[rosstimothy]

I would remove this and make the call directly inline to be consistent with every other invocation in this file.

[rosstimothy]

Let's add a comment similar to the one in ListResources to provide some context on why this is needed.

[rosstimothy]

Suggestion: reduce the scope of errors where possible

Suggested change

	err := a.quietAction(apidefaults.Namespace, resourceKind, actionVerbs...)
	if err != nil {
	if err := a.quietAction(apidefaults.Namespace, resourceKind, actionVerbs...); err != nil {

[rosstimothy]

Suggestion: reduce the scope of errors where possible

Suggested change

	err = checker.CanAccess(resource)
	if err != nil {
	if err := checker.CanAccess(resource); err != nil {

[rosstimothy]

nit: missing a trace.Wrap on returned errors here

Suggested change

	return services.MatchResourceByFilters(resource, filter, nil)
	match, err := services.MatchResourceByFilters(resource, filter, nil)
	return match, trace.Wrap(err)

[rosstimothy]

Backend changes look good to me. Could we just add a test that covers this?

[rosstimothy]

Suggested change

	return a.checkUnifiedAccess(resource, checker, filter, resourceAccessMap)
	match, err := a.checkUnifiedAccess(resource, checker, filter, resourceAccessMap)
	return match, trace.Wrap(err)

[rosstimothy]

Suggested change

	return a.checkUnifiedAccess(resource, checker, filter, resourceAccessMap)
	match, err := a.checkUnifiedAccess(resource, checker, filter, resourceAccessMap)
	return match, trace.Wrap(err)

[kimlisa]

lgtm 👍, after tim's comment

[public-teleport-github-review-bot]

@avatus See the table below for backport results.

Branch	Result
branch/v14	Failed