Clean up system role parsing (#9756)

* Add tests for ParseTeleportRoles

Updates #9752

* Be more tolerant when parsing system roles.

Our original attempt at canonicalizing roles didn't work for system
roles using camelcase, resulting in an awkward user experience.

Here we maintain a mapping of allowed inputs to their corresponding
system roles, and perform a case-insensitive lookup. This allows us
to support camelcase roles, and has the advantage of permitting
_ word separators as well.

Fixes #9752

* Refactor *SystemRole.Check()

Rather than having to list each role here, we rely on the new
roleMappings set to validate the role.

Additionally, remove the LegacyClusterTokenType role. This change
is guaranteed to be backwards compatible because we check for
RoleTrustedCluster everywhere we were checking for
LegacyClusterTokenType, and our roleMappings will convert the old
string that represented LegacyClusterTokenType to RoleTrustedCluster.

api/types/system_role.go

@@ -17,7 +17,6 @@ limitations under the License.
 package types
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/gravitational/trace"
@@ -61,8 +60,26 @@ const (
 	RoleWindowsDesktop SystemRole = "WindowsDesktop"
 )
 
-// LegacyClusterTokenType exists for backwards compatibility reasons, needed to upgrade to 2.3
-const LegacyClusterTokenType SystemRole = "Trustedcluster"
+// roleMappings maps a set of allowed lowercase system role names
+// to the proper system role
+var roleMappings = map[string]SystemRole{
+	"auth":            RoleAuth,
+	"node":            RoleNode,
+	"proxy":           RoleProxy,
+	"admin":           RoleAdmin,
+	"provisiontoken":  RoleProvisionToken,
+	"trusted_cluster": RoleTrustedCluster,
+	"trustedcluster":  RoleTrustedCluster,
+	"signup":          RoleSignup,
+	"nop":             RoleNop,
+	"remoteproxy":     RoleRemoteProxy,
+	"remote_proxy":    RoleRemoteProxy,
+	"kube":            RoleKube,
+	"app":             RoleApp,
+	"db":              RoleDatabase,
+	"windowsdesktop":  RoleWindowsDesktop,
+	"windows_desktop": RoleWindowsDesktop,
+}
 
 // NewTeleportRoles return a list of teleport roles from slice of strings
 func NewTeleportRoles(in []string) (SystemRoles, error) {
@@ -78,15 +95,17 @@ func NewTeleportRoles(in []string) (SystemRoles, error) {
 func ParseTeleportRoles(str string) (SystemRoles, error) {
 	var roles SystemRoles
 	for _, s := range strings.Split(str, ",") {
-		s = strings.TrimSpace(s)
-		if r := SystemRole(s); r.Check() == nil {
+		cleaned := strings.ToLower(strings.TrimSpace(s))
+		if r, ok := roleMappings[cleaned]; ok && r.Check() == nil {
 			roles = append(roles, r)
 			continue
 		}
-		// If role is not valid as-is, attempt to canonicalize the format.
-		r := SystemRole(strings.Title(strings.ToLower(s)))
-		roles = append(roles, r)
+		return nil, trace.BadParameter("invalid role %q", s)
 	}
+	if len(roles) == 0 {
+		return nil, trace.BadParameter("no valid roles in $%q", str)
+	}
+
 	return roles, roles.Check()
 }
 
@@ -178,22 +197,22 @@ func (r *SystemRole) String() string {
 	switch *r {
 	case RoleSignup:
 		return "Password"
-	case RoleTrustedCluster, LegacyClusterTokenType:
+	case RoleTrustedCluster:
 		return "trusted_cluster"
 	default:
-		return fmt.Sprintf("%v", string(*r))
+		return string(*r)
 	}
 }
 
+// Check checks if this a a valid teleport role value, returns nil
+// if it's ok, false otherwise
 // Check checks if this a a valid teleport role value, returns nil
 // if it's ok, false otherwise
 func (r *SystemRole) Check() error {
-	switch *r {
-	case RoleAuth, RoleNode, RoleApp, RoleDatabase,
-		RoleAdmin, RoleProvisionToken,
-		RoleTrustedCluster, LegacyClusterTokenType,
-		RoleSignup, RoleProxy, RoleNop, RoleKube, RoleWindowsDesktop:
+	sr, ok := roleMappings[strings.ToLower(string(*r))]
+	if ok && string(*r) == string(sr) {
 		return nil
 	}
+
 	return trace.BadParameter("role %v is not registered", *r)
 }

api/types/system_role_test.go

@@ -17,6 +17,8 @@ package types
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestRolesCheck(t *testing.T) {
@@ -26,7 +28,7 @@ func TestRolesCheck(t *testing.T) {
 	}{
 		{roles: []SystemRole{}, wantErr: false},
 		{roles: []SystemRole{RoleAuth}, wantErr: false},
-		{roles: []SystemRole{RoleAuth, RoleNode, RoleProxy, RoleAdmin, RoleProvisionToken, RoleTrustedCluster, LegacyClusterTokenType, RoleSignup, RoleNop}, wantErr: false},
+		{roles: []SystemRole{RoleAuth, RoleNode, RoleProxy, RoleAdmin, RoleProvisionToken, RoleTrustedCluster, RoleSignup, RoleNop}, wantErr: false},
 		{roles: []SystemRole{RoleAuth, RoleNode, RoleAuth}, wantErr: true},
 		{roles: []SystemRole{SystemRole("unknown")}, wantErr: true},
 	}
@@ -62,3 +64,77 @@ func TestRolesEqual(t *testing.T) {
 		}
 	}
 }
+
+func TestParseTeleportRoles(t *testing.T) {
+	t.Parallel()
+	for _, test := range []struct {
+		in      string
+		out     SystemRoles
+		wantErr bool
+	}{
+		{
+			// system role constant
+			in:      string(RoleNode),
+			out:     SystemRoles{RoleNode},
+			wantErr: false,
+		},
+		{
+			// alternate capitalization
+			in:      "node",
+			out:     SystemRoles{RoleNode},
+			wantErr: false,
+		},
+		{
+			in:      "windowsdesktop",
+			out:     SystemRoles{RoleWindowsDesktop},
+			wantErr: false,
+		},
+		{
+			// allow underscores for camelcase roles
+			in:      "windows_desktop,trusted_cluster,remote_proxy",
+			out:     SystemRoles{RoleWindowsDesktop, RoleTrustedCluster, RoleRemoteProxy},
+			wantErr: false,
+		},
+		{
+			// multiple comma-separated roles
+			in:      "Auth,Proxy",
+			out:     SystemRoles{RoleAuth, RoleProxy},
+			wantErr: false,
+		},
+		{
+			// extra whitespace handled properly
+			in:      "Auth , Proxy,WindowsDesktop",
+			out:     SystemRoles{RoleAuth, RoleProxy, RoleWindowsDesktop},
+			wantErr: false,
+		},
+		{
+			// duplicate roles is an error
+			in:      "Auth,Auth",
+			wantErr: true,
+		},
+		{
+			in:      "Auth, auth",
+			wantErr: true,
+		},
+		{
+			// invalid role errors
+			in:      "invalidrole",
+			wantErr: true,
+		},
+		{
+			// valid + invalid role errors
+			in:      "Admin,invalidrole",
+			wantErr: true,
+		},
+	} {
+		t.Run(test.in, func(t *testing.T) {
+			roles, err := ParseTeleportRoles(test.in)
+			if test.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, test.out, roles)
+			}
+		})
+	}
+}