handle code review comments

tool/tctl/common/plugin/entraid.go

@@ -43,6 +43,33 @@ import (
 	"github.com/gravitational/teleport/lib/web/scripts/oneoff"
 )
 
+var (
+	bold    = color.New(color.Bold).SprintFunc()
+	boldRed = color.New(color.Bold, color.FgRed).SprintFunc()
+
+	step1Template = `Step 1: Run the Setup Script
+
+1. Open ` + bold("Azure Cloud Shell") + ` (Bash) [https://portal.azure.com/#cloudshell/] using ` + bold("Google Chrome") + ` or ` + bold("Safari") + ` for the best compatibility.
+2. Upload the setup script in ` + boldRed("%s") + ` using the ` + bold("Upload") + ` button in the Cloud Shell toolbar.
+3. Once uploaded, execute the script by running the following command:
+   $ bash %s
+
+` + bold("Important Considerations") + `:
+- You must have ` + bold("Azure privileged administrator permissions") + ` to complete the integration.
+- Ensure you're using the ` + bold("Bash") + ` environment in Cloud Shell.
+- During the script execution, you'll be prompted to run 'az login' to authenticate with Azure. ` + bold("Teleport") + ` does not store or persist your credentials.
+- ` + bold("Mozilla Firefox") + ` users may experience connectivity issues in Azure Cloud Shell; using Chrome or Safari is recommended.
+
+`
+
+	step2Template = `
+	
+Step 2: Input Tenant ID and Client ID
+
+With the output of Step 1, please copy and paste the following information:
+`
+)
+
 type entraArgs struct {
 	cmd                  *kingpin.CmdClause
 	authConnectorName    string
@@ -107,7 +134,7 @@ func (p *PluginsCommand) entraSetupGuide(proxyPublicAddr string) (entraSettings,
 
 	defer os.Remove(f.Name())
 
-	buildScript, err := buildScript(proxyPublicAddr, p.install.entraID.authConnectorName, p.install.entraID.accessGraph, p.install.entraID.useSystemCredentials)
+	buildScript, err := buildScript(proxyPublicAddr, p.install.entraID)
 	if err != nil {
 		return entraSettings{}, trace.Wrap(err, "failed to build script")
 	}
@@ -121,28 +148,10 @@ func (p *PluginsCommand) entraSetupGuide(proxyPublicAddr string) (entraSettings,
 	}
 	fileLoc := f.Name()
 
-	bold := color.New(color.Bold).SprintFunc()
-	boldRed := color.New(color.Bold, color.FgRed).SprintFunc()
-
-	tmpl := `Step 1: Run the Setup Script
-
-1. Open ` + bold("Azure Cloud Shell") + ` (Bash) [https://portal.azure.com/#cloudshell/] using ` + bold("Google Chrome") + ` or ` + bold("Safari") + ` for the best compatibility.
-2. Upload the setup script in ` + boldRed(fileLoc) + ` using the ` + bold("Upload") + ` button in the Cloud Shell toolbar.
-3. Once uploaded, execute the script by running the following command:
-   $ bash %s
-
-` + bold("Important Considerations") + `:
-- You must have ` + bold("Azure privileged administrator permissions") + ` to complete the integration.
-- Ensure you're using the ` + bold("Bash") + ` environment in Cloud Shell.
-- During the script execution, you'll be prompted to run 'az login' to authenticate with Azure. ` + bold("Teleport") + ` does not store or persist your credentials.
-- ` + bold("Mozilla Firefox") + ` users may experience connectivity issues in Azure Cloud Shell; using Chrome or Safari is recommended.
-
-`
-
-	fmt.Fprintf(os.Stdout, tmpl, filepath.Base(fileLoc))
+	fmt.Fprintf(os.Stdout, step1Template, fileLoc, filepath.Base(fileLoc))
 
 	op, err := readData(os.Stdin, os.Stdout,
-		"Once the script completes, type 'continue' to proceed, 'exit' to quit",
+		"Once the script completes, type 'continue' to proceed, 'exit' to quit. If you need to rerun the script, type 'exit' and restart the process.",
 		func(input string) bool {
 			return input == "continue" || input == "exit"
 		}, "Invalid input. Please enter 'continue' or 'exit'.")
@@ -158,13 +167,8 @@ func (p *PluginsCommand) entraSetupGuide(proxyPublicAddr string) (entraSettings,
 		return err == nil
 	}
 
-	tmpl = `
-	
-Step 2: Input Tenant ID and Client ID
+	fmt.Fprint(os.Stdout, step2Template)
 
-With the output of Step 1, please copy and paste the following information:
-`
-	fmt.Fprint(os.Stdout, tmpl)
 	var settings entraSettings
 	settings.tenantID, err = readData(os.Stdin, os.Stdout, "Enter the Tenant ID", validUUID, "Invalid Tenant ID")
 	if err != nil {
@@ -232,12 +236,11 @@ func (p *PluginsCommand) InstallEntra(ctx context.Context, args installPluginArg
 		AssertionConsumerService: strings.TrimRight(proxyPublicAddr, "/") + "/v1/webapi/saml/acs/" + inputs.entraID.authConnectorName,
 		AllowIDPInitiated:        true,
 		// AttributesToRoles is required, but Entra ID does not have a default group (like Okta's "Everyone"),
-		// so we add a dummy claim that will never be fulfilled with the default configuration instead,
-		// and expect the user to modify it per their requirements.
+		// so we add a dummy claim that will always be fulfilled and map them to the "requester" role.
 		AttributesToRoles: []types.AttributeMapping{
 			{
-				Name:  "https://example.com/my_attribute",
-				Value: "my_value",
+				Name:  "http://schemas.microsoft.com/ws/2008/06/identity/claims/groups",
+				Value: "*",
 				Roles: []string{"requester"},
 			},
 		},
@@ -273,10 +276,13 @@ func (p *PluginsCommand) InstallEntra(ctx context.Context, args installPluginArg
 			if !trace.IsAlreadyExists(err) || !inputs.entraID.force {
 				return trace.Wrap(err, "failed to create Azure OIDC integration")
 			}
-			if err = args.authClient.DeleteIntegration(ctx, integrationSpec.GetName()); err != nil {
-				return trace.Wrap(err, "failed to delete Azure OIDC integration")
+
+			integration, err := args.authClient.GetIntegration(ctx, integrationSpec.GetName())
+			if err != nil {
+				return trace.Wrap(err, "failed to get Azure OIDC integration")
 			}
-			if _, err = args.authClient.CreateIntegration(ctx, integrationSpec); err != nil {
+			integration.SetAWSOIDCIntegrationSpec(integrationSpec.GetAWSOIDCIntegrationSpec())
+			if _, err = args.authClient.UpdateIntegration(ctx, integration); err != nil {
 				return trace.Wrap(err, "failed to create Azure OIDC integration")
 			}
 		}
@@ -315,12 +321,19 @@ func (p *PluginsCommand) InstallEntra(ctx context.Context, args installPluginArg
 		if !trace.IsAlreadyExists(err) || !inputs.entraID.force {
 			return trace.Wrap(err)
 		}
-		if _, err = args.plugins.DeletePlugin(ctx, &pluginspb.DeletePluginRequest{
-			Name: inputs.name,
-		}); err != nil {
-			return trace.Wrap(err)
+		plugin := req.GetPlugin()
+		{
+			oldPlugin, err := args.plugins.GetPlugin(ctx, &pluginspb.GetPluginRequest{
+				Name: inputs.name,
+			})
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			plugin.Metadata.Revision = oldPlugin.GetMetadata().Revision
 		}
-		if _, err = args.plugins.CreatePlugin(ctx, req); err != nil {
+		if _, err = args.plugins.UpdatePlugin(ctx, &pluginspb.UpdatePluginRequest{
+			Plugin: plugin,
+		}); err != nil {
 			return trace.Wrap(err)
 		}
 	}
@@ -330,19 +343,19 @@ func (p *PluginsCommand) InstallEntra(ctx context.Context, args installPluginArg
 	return nil
 }
 
-func buildScript(proxyPublicAddr string, authConnectorName string, accessGraph, skipOIDCSetup bool) (string, error) {
+func buildScript(proxyPublicAddr string, entraCfg entraArgs) (string, error) {
 	// The script must execute the following command:
 	argsList := []string{
 		"integration", "configure", "azure-oidc",
 		fmt.Sprintf("--proxy-public-addr=%s", shsprintf.EscapeDefaultContext(proxyPublicAddr)),
-		fmt.Sprintf("--auth-connector-name=%s", shsprintf.EscapeDefaultContext(authConnectorName)),
+		fmt.Sprintf("--auth-connector-name=%s", shsprintf.EscapeDefaultContext(entraCfg.authConnectorName)),
 	}
 
-	if accessGraph {
+	if entraCfg.accessGraph {
 		argsList = append(argsList, "--access-graph")
 	}
 
-	if skipOIDCSetup {
+	if entraCfg.useSystemCredentials {
 		argsList = append(argsList, "--skip-oidc-integration")
 	}
 
@@ -366,6 +379,9 @@ func getProxyPublicAddr(ctx context.Context, authClient authClient) (string, err
 	return oidcIssuer, trace.Wrap(err)
 }
 
+// readTAGCache reads the TAG cache file and returns the TAGInfoCache object.
+// azureoidc.TAGInfoCache is a struct that contains the information necessary for Access Graph to analyze Azure SSO.
+// It contains a list of AppID and their corresponding FederatedSsoV2 information.
 func readTAGCache(fileLoc string) (*azureoidc.TAGInfoCache, error) {
 	if fileLoc == "" {
 		return nil, trace.BadParameter("no TAG cache file specified")

tool/tctl/common/plugin/plugins_command.go

@@ -205,13 +205,15 @@ type authClient interface {
 	CreateSAMLConnector(ctx context.Context, connector types.SAMLConnector) (types.SAMLConnector, error)
 	UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) (types.SAMLConnector, error)
 	CreateIntegration(ctx context.Context, ig types.Integration) (types.Integration, error)
-	DeleteIntegration(ctx context.Context, name string) error
+	GetIntegration(ctx context.Context, name string) (types.Integration, error)
+	UpdateIntegration(ctx context.Context, ig types.Integration) (types.Integration, error)
 	Ping(ctx context.Context) (proto.PingResponse, error)
 }
 
 type pluginsClient interface {
 	CreatePlugin(ctx context.Context, in *pluginsv1.CreatePluginRequest, opts ...grpc.CallOption) (*emptypb.Empty, error)
-	DeletePlugin(ctx context.Context, in *pluginsv1.DeletePluginRequest, opts ...grpc.CallOption) (*emptypb.Empty, error)
+	GetPlugin(ctx context.Context, in *pluginsv1.GetPluginRequest, opts ...grpc.CallOption) (*types.PluginV1, error)
+	UpdatePlugin(ctx context.Context, in *pluginsv1.UpdatePluginRequest, opts ...grpc.CallOption) (*types.PluginV1, error)
 }
 
 type installPluginArgs struct {

tool/tctl/common/plugin/plugins_command_test.go

@@ -449,9 +449,14 @@ func (m *mockPluginsClient) CreatePlugin(ctx context.Context, in *pluginsv1.Crea
 	return result.Get(0).(*emptypb.Empty), result.Error(1)
 }
 
-func (m *mockPluginsClient) DeletePlugin(ctx context.Context, in *pluginsv1.DeletePluginRequest, opts ...grpc.CallOption) (*emptypb.Empty, error) {
+func (m *mockPluginsClient) GetPlugin(ctx context.Context, in *pluginsv1.GetPluginRequest, opts ...grpc.CallOption) (*types.PluginV1, error) {
 	result := m.Called(ctx, in, opts)
-	return result.Get(0).(*emptypb.Empty), result.Error(1)
+	return result.Get(0).(*types.PluginV1), result.Error(1)
+}
+
+func (m *mockPluginsClient) UpdatePlugin(ctx context.Context, in *pluginsv1.UpdatePluginRequest, opts ...grpc.CallOption) (*types.PluginV1, error) {
+	result := m.Called(ctx, in, opts)
+	return result.Get(0).(*types.PluginV1), result.Error(1)
 }
 
 type mockAuthClient struct {
@@ -474,10 +479,16 @@ func (m *mockAuthClient) CreateIntegration(ctx context.Context, ig types.Integra
 	result := m.Called(ctx, ig)
 	return result.Get(0).(types.Integration), result.Error(1)
 }
-func (m *mockAuthClient) DeleteIntegration(ctx context.Context, name string) error {
+func (m *mockAuthClient) UpdateIntegration(ctx context.Context, ig types.Integration) (types.Integration, error) {
+	result := m.Called(ctx, ig)
+	return result.Get(0).(types.Integration), result.Error(1)
+}
+
+func (m *mockAuthClient) GetIntegration(ctx context.Context, name string) (types.Integration, error) {
 	result := m.Called(ctx, name)
-	return result.Error(0)
+	return result.Get(0).(types.Integration), result.Error(1)
 }
+
 func (m *mockAuthClient) Ping(ctx context.Context) (proto.PingResponse, error) {
 	result := m.Called(ctx)
 	return result.Get(0).(proto.PingResponse), result.Error(1)