Auth: conflicting users validation improvements (#58136)

* WIP

* add: better validation of conflict file

* add: better description of validation and ingest command

* add: check for at least one user to delete

* add: example in terraform to setup for conflicts

* Update pkg/cmd/grafana-cli/commands/conflict_user_command.go

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

* Add: print of conflict block for error

- adds conflict block to error output for validation of the file to
  easier diagnose in the file

* fix: formatting of errors

* fix: info strings improvements

* add: default 0 to blocks to check for users

* fixed: tests

* test integration

* fix strings fmt

* set store in resolver

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

pkg/cmd/grafana-cli/commands/commands.go

@@ -219,7 +219,7 @@ var adminCommands = []*cli.Command{
 					},
 					{
 						Name:   "ingest-file",
-						Usage:  "ingests the conflict users file",
+						Usage:  "ingests the conflict users file. > Note: This is irreversible it will change the state of the database.",
 						Action: runIngestConflictUsersFile(),
 					},
 				},

pkg/cmd/grafana-cli/commands/conflict-examples/conflict_example_users.tf

@@ -0,0 +1,64 @@
+terraform {
+  required_providers {
+    grafana = {
+      source  = "grafana/grafana"
+    }
+  }
+}
+
+// Configure the Grafana Provider
+provider "grafana" {
+  url  = "http://localhost:3000/"
+  auth = "admin:admin"
+}
+
+// login conflict
+// Creating the grafana-login
+resource "grafana_user" "grafana-login" {
+  email    = "grafana_login@grafana.com"
+  login    = "GRAFANA_LOGIN@grafana.com"
+  password = "grafana_login@grafana.com"
+  is_admin = false
+}
+
+// Creating the grafana-login
+resource "grafana_user" "grafana-login-2" {
+  email    = "grafana_login_2@grafana.com"
+  login    = "grafana_login@grafana.com"
+  password = "grafana_login@grafana.com"
+  is_admin = false
+}
+
+// email conflict
+// Creating the grafana-email
+resource "grafana_user" "grafana-email" {
+  email    = "grafana_email@grafana.com"
+  login    = "grafana_email@grafana.com"
+  password = "grafana_email@grafana.com"
+  is_admin = false
+}
+
+// Creating the grafana-email
+resource "grafana_user" "grafana-email-2" {
+  email    = "GRAFANA_EMAIL@grafana.com"
+  login    = "grafana_email_2@grafana.com"
+  password = "grafana_email@grafana.com"
+  is_admin = false
+}
+
+// email and login conflict
+// Creating the grafana-user
+resource "grafana_user" "grafana-user" {
+  email    = "grafana_user@grafana.com"
+  login    = "grafana_user@grafana.com"
+  password = "grafana_user@grafana.com"
+  is_admin = false
+}
+
+// Creating the grafana-user
+resource "grafana_user" "grafana-user-2" {
+  email    = "GRAFANA_USER@grafana.com"
+  login    = "GRAFANA_USER@grafana.com"
+  password = "grafana_user@grafana.com"
+  is_admin = false
+}

pkg/cmd/grafana-cli/commands/conflict_user_command.go

@@ -9,6 +9,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"unicode"
 
 	"github.com/fatih/color"
 	"github.com/urfave/cli/v2"
@@ -54,7 +55,7 @@ func initializeConflictResolver(cmd *utils.ContextCommandLine, f Formatter, ctx
 	if err != nil {
 		return nil, fmt.Errorf("%v: %w", "failed to get users with conflicting logins", err)
 	}
-	resolver := ConflictResolver{Users: conflicts}
+	resolver := ConflictResolver{Users: conflicts, Store: s}
 	resolver.BuildConflictBlocks(conflicts, f)
 	return &resolver, nil
 }
@@ -127,17 +128,20 @@ func runValidateConflictUsersFile() func(context *cli.Context) error {
 		// read in the file to ingest
 		arg := cmd.Args().First()
 		if arg == "" {
-			return errors.New("please specify a absolute path to file to read from")
+			return fmt.Errorf("please specify a absolute path to file to read from")
 		}
 		b, err := os.ReadFile(filepath.Clean(arg))
 		if err != nil {
-			return fmt.Errorf("could not read file with error %e", err)
+			logger.Error(color.RedString("validation failed with an error"))
+			return fmt.Errorf("could not read file with error %s", err)
 		}
 		validErr := getValidConflictUsers(r, b)
 		if validErr != nil {
-			return fmt.Errorf("could not validate file with error %s", err)
+			logger.Error(color.RedString("validation failed with an error"))
+			return fmt.Errorf("could not validate file with error:\n%s", validErr)
 		}
-		logger.Info("File validation complete without errors.\n\n File can be used with ingesting command `ingest-file`.\n\n")
+		logger.Info(color.GreenString("File validation complete.\n"))
+		logger.Info("File can be used with the `ingest-file` command.\n\n")
 		return nil
 	}
 }
@@ -161,7 +165,7 @@ func runIngestConflictUsersFile() func(context *cli.Context) error {
 		}
 		validErr := getValidConflictUsers(r, b)
 		if validErr != nil {
-			return fmt.Errorf("could not validate file with error %s", validErr)
+			return fmt.Errorf("could not validate file with error:\n%s", validErr)
 		}
 		// should we rebuild blocks here?
 		// kind of a weird thing maybe?
@@ -169,7 +173,7 @@ func runIngestConflictUsersFile() func(context *cli.Context) error {
 			return fmt.Errorf("no users")
 		}
 		r.showChanges()
-		if !confirm("\n\nWe encourage users to create a db backup before running this command. \n Proceed with operation?") {
+		if !confirm("\n\nWe encourage users to create a db backup before running this command. \n Proceed with operation") {
 			return fmt.Errorf("user cancelled")
 		}
 		err = r.MergeConflictingUsers(context.Context)
@@ -230,7 +234,6 @@ func getValidConflictUsers(r *ConflictResolver, b []byte) error {
 			previouslySeenLogins[strings.ToLower(u.Login)] = true
 		}
 	}
-
 	// tested in https://regex101.com/r/una3zC/1
 	diffPattern := `^[+-]`
 	// compiling since in a loop
@@ -238,23 +241,50 @@ func getValidConflictUsers(r *ConflictResolver, b []byte) error {
 	if err != nil {
 		return fmt.Errorf("unable to compile regex %s: %w", diffPattern, err)
 	}
-	for _, row := range strings.Split(string(b), "\n") {
+	counterKeepUsersForBlock := map[string]int{}
+	counterDeleteUsersForBlock := map[string]int{}
+	currentBlock := ""
+	for rowNumber, row := range strings.Split(string(b), "\n") {
+		// end of file
 		if row == "" {
-			// end of file
 			break
 		}
 		// if the row starts with a #, it is a comment
 		if row[0] == '#' {
-			// comment
-			continue
-		}
-		entryRow := matchingExpression.Match([]byte(row))
-		if !entryRow {
-			// block row
-			// conflict: hej
 			continue
 		}
 
+		entryRow := matchingExpression.Match([]byte(row))
+		// not an entry row -> is a conflict block row
+		if !entryRow {
+			// check for malformed row
+			// rows should be of the form
+			// conflict: <conflict>
+			// or
+			// + id: <id>
+			// - id: <id>
+			if (row[0] != '-') && (row[0] != '+') && (row[0] != 'c') {
+				return fmt.Errorf("invalid start character (expected '+,-') found %c for row number %d", row[0], rowNumber+1)
+			}
+
+			// is a conflict block row
+			// conflict: hej
+			currentBlock = row
+			continue
+		}
+		// need to track how many keep users we have for a block
+		if _, ok := counterKeepUsersForBlock[currentBlock]; !ok {
+			counterKeepUsersForBlock[currentBlock] = 0
+		}
+		if _, ok := counterDeleteUsersForBlock[currentBlock]; !ok {
+			counterDeleteUsersForBlock[currentBlock] = 0
+		}
+		if row[0] == '+' {
+			counterKeepUsersForBlock[currentBlock] += 1
+		}
+		if row[0] == '-' {
+			counterDeleteUsersForBlock[currentBlock] += 1
+		}
 		newUser := &ConflictingUser{}
 		err := newUser.Marshal(row)
 		if err != nil {
@@ -269,6 +299,18 @@ func getValidConflictUsers(r *ConflictResolver, b []byte) error {
 		// valid entry
 		newConflicts = append(newConflicts, *newUser)
 	}
+	for block, count := range counterKeepUsersForBlock {
+		// check if we only have one addition for each block
+		if count != 1 {
+			return fmt.Errorf("invalid number of users to keep, expected 1, got %d for block: %s", count, block)
+		}
+	}
+	for block, count := range counterDeleteUsersForBlock {
+		// check if we have at least one deletion for each block
+		if count < 1 {
+			return fmt.Errorf("invalid number of users to delete, should be at least 1, got %d for block %s", count, block)
+		}
+	}
 	r.ValidUsers = newConflicts
 	r.BuildConflictBlocks(newConflicts, fmt.Sprintf)
 	return nil
@@ -378,7 +420,14 @@ func (r *ConflictResolver) showChanges() {
 		}
 		b.WriteString("Keep the following user.\n")
 		b.WriteString(fmt.Sprintf("%s\n", block))
-		b.WriteString(fmt.Sprintf("id: %s, email: %s, login: %s\n", mainUser.ID, mainUser.Email, mainUser.Login))
+		b.WriteString(color.GreenString(fmt.Sprintf("id: %s, email: %s, login: %s\n", mainUser.ID, mainUser.Email, mainUser.Login)))
+		for _, r := range fmt.Sprintf("%s%s", mainUser.Email, mainUser.Login) {
+			if unicode.IsUpper(r) {
+				b.WriteString("Will be change to:\n")
+				b.WriteString(color.GreenString(fmt.Sprintf("id: %s, email: %s, login: %s\n", mainUser.ID, strings.ToLower(mainUser.Email), strings.ToLower(mainUser.Login))))
+				break
+			}
+		}
 		b.WriteString("\n\n")
 		b.WriteString("The following user(s) will be deleted.\n")
 		for _, user := range users {
@@ -386,7 +435,7 @@ func (r *ConflictResolver) showChanges() {
 				continue
 			}
 			// mergeable users
-			b.WriteString(fmt.Sprintf("id: %s, email: %s, login: %s\n", user.ID, user.Email, user.Login))
+			b.WriteString(color.RedString(fmt.Sprintf("id: %s, email: %s, login: %s\n", user.ID, user.Email, user.Login)))
 		}
 		b.WriteString("\n\n")
 	}

pkg/cmd/grafana-cli/commands/conflict_user_command_test.go

@@ -577,7 +577,7 @@ func TestRunValidateConflictUserFile(t *testing.T) {
 	})
 }
 
-func TestMergeUser(t *testing.T) {
+func TestIntegrationMergeUser(t *testing.T) {
 	t.Run("should be able to merge user", func(t *testing.T) {
 		// Restore after destructive operation
 		sqlStore := db.InitTestDB(t)
@@ -632,17 +632,15 @@ func TestMergeUser(t *testing.T) {
 	})
 }
 
-func TestMergeUserFromNewFileInput(t *testing.T) {
+func TestIntegrationMergeUserFromNewFileInput(t *testing.T) {
 	t.Run("should be able to merge users after choosing a different user to keep", func(t *testing.T) {
-		// Restore after destructive operation
-		sqlStore := db.InitTestDB(t)
-
 		type testBuildConflictBlock struct {
-			desc                string
+			desc                  string
-			users               []user.User
+			users                 []user.User
-			fileString          string
+			fileString            string
-			expectedBlocks      []string
+			expectedValidationErr error
-			expectedIdsInBlocks map[string][]string
+			expectedBlocks        []string
+			expectedIdsInBlocks   map[string][]string
 		}
 		testOrgID := 1
 		m := make(map[string][]string)
@@ -690,8 +688,52 @@ conflict: test2
 				expectedBlocks:      []string{"conflict: test", "conflict: test2"},
 				expectedIdsInBlocks: m,
 			},
+			{
+				desc: "should give error for having wrong number of users to keep",
+				users: []user.User{
+					{
+						Email: "TEST",
+						Login: "TEST",
+						OrgID: int64(testOrgID),
+					},
+					{
+						Email: "test",
+						Login: "test",
+						OrgID: int64(testOrgID),
+					},
+				},
+				fileString: `conflict: test
++ id: 1, email: test, login: test, last_seen_at: 2012-09-19T08:31:20Z, auth_module:, conflict_email: true, conflict_login: true
++ id: 2, email: TEST, login: TEST, last_seen_at: 2012-09-19T08:31:29Z, auth_module:, conflict_email: true, conflict_login: true
+`,
+				expectedValidationErr: fmt.Errorf("invalid number of users to keep, expected 1, got 2 for block: conflict: test"),
+				expectedBlocks:        []string{"conflict: test"},
+			},
+			{
+				desc: "should give error for having wrong character for user",
+				users: []user.User{
+					{
+						Email: "TEST",
+						Login: "TEST",
+						OrgID: int64(testOrgID),
+					},
+					{
+						Email: "test",
+						Login: "test",
+						OrgID: int64(testOrgID),
+					},
+				},
+				fileString: `conflict: test
++ id: 1, email: test, login: test, last_seen_at: 2012-09-19T08:31:20Z, auth_module:, conflict_email: true, conflict_login: true
+% id: 2, email: TEST, login: TEST, last_seen_at: 2012-09-19T08:31:29Z, auth_module:, conflict_email: true, conflict_login: true
+`,
+				expectedValidationErr: fmt.Errorf("invalid start character (expected '+,-') found %% for row number 3"),
+				expectedBlocks:        []string{"conflict: test"},
+			},
 		}
 		for _, tc := range testCases {
+			// Restore after destructive operation
+			sqlStore := db.InitTestDB(t)
 			if sqlStore.GetDialect().DriverName() != ignoredDatabase {
 				for _, u := range tc.users {
 					cmd := user.CreateUserCommand{
@@ -716,7 +758,11 @@ conflict: test2
 				b := tc.fileString
 				require.NoError(t, err)
 				validErr := getValidConflictUsers(&r, []byte(b))
-				require.NoError(t, validErr)
+				if tc.expectedValidationErr != nil {
+					require.Equal(t, tc.expectedValidationErr, validErr)
+				} else {
+					require.NoError(t, validErr)
+				}
 
 				// test starts here
 				err = r.MergeConflictingUsers(context.Background())