Remove explicit "deny" from preset "auditor" role, make preset roles V4

[justinas]

This PR intends to make the auditor purely additive and bring the combination of built-in access,auditor,editor roles up to par with the legacy admin. The two main goals of this:

1. In Cloud, we want the initial user to have access to all the features, this allows to achieve it easily with the combination of the three roles.
2. In any other environment, we would like users to have a good first experience and to be able to try out all features. Adding auditor role to the account in order to try out session replay and losing permissions to the infra itself is a confusing outcome.

Previously, the auditor role would explicitly deny access to infrastructure, and as such combining it with access took away the permissions granted by the access role. This made sense as v3 roles had lax allow rules by default (node_labels: {'*': '*'}). However, v4 roles do not allow anything implicitly (see Roles V4 PR).

By this reasoning, removing deny together with moving the roles to v4 should be safe (i.e. not give auditor extra permissions), while removing its negative interplay with other built-in roles.

Additional impact of this change:

1. Because editor role does not grant access to compute infrastructure by default, the editor role by itself is not sufficient to edit nodes, DBs, and other labelled resources. The user must be given access,editor roles to be able to edit these objects.

[klizhentas]

LGTM. I think auditor role could be a bit more permissive without loosing the value.

[nklaassen]

Looks like the editor role will lose wildcard allow rules for nodes, k8s, etc. Not sure if this is intentional

[justinas]

I think that makes sense, the intention of these roles seems to be to grant distinct, limited sets of permissions each. If you'd like access, you should create a user with access,editor. A "superuser" would be one with all three roles.

[nklaassen]

It probably does make sense I just think we should be careful/intentional with this change as it may break some user's expectations of the editor role (even though this will not affect existing clusters, it could break expectations for new clusters).

Can we confirm this won't break the editor's ability to edit any labelled resource? I'm not sure if that's even something you can do but worth double checking.

With 8.0 we are already changing admin/auditor so it probably is the time to make this change but I think this is at least worth calling out in the PR description and getting UX sign-off

[justinas]

@nklaassen you're right, with this change editor role on its own does not allow editing nodes, DBs, etc. Not sure whether should be this case, the description of editor in the docs is somewhat vague:

Allows editing of cluster configuration settings.
https://goteleport.com/docs/ver/8.0/access-controls/reference/#preset-roles

---

Alternatively, we could leave these roles on V3, only remove explicit deny on auditor. This means auditor remains with insecure defaults (wildcard labels), but nologin-FOO and the absence of {{internal.logins}} should prevent from accidentally granting infra access to auditors.

[Joerger]

I think to edit nodes, DBs, etc., requiring users to have access,editor makes the most sense and keeps things nicely modular. Whether we slowly deprecate this or remove the default labels now doesn't make much of a difference since it won't affect current clusters and users will have to adapt to these fixed presets sooner or later.

So I agree, let's update the PR description to include this hidden change and get it signed off.

[justinas]

@Joerger thanks for your 2¢, I have updated the description, but I'm not very familiar with the UX review process, could you tag the key people for a review?

[nklaassen]

IMO we should still update the editor role to v4. We can either add the explicit wildcard allow labels to keep the existing functionality, or leave them off if we intentionally want the editor role to be more restricted, I'm not sure which is better but I would view adding the wildcard allow labels as the default, non-change. If we want to remove them that may be valid but is kind of a separate issue. But I think it's fine either way. Last time I needed UX review it's just @klizhentas