From 76947b10e2cae93daef38a88c3c7db1b6a197f85 Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Mon, 7 Nov 2022 18:12:17 +0000 Subject: [PATCH] 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 * 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 --- pkg/cmd/grafana-cli/commands/commands.go | 2 +- .../conflict_example_users.tf | 64 ++++++++++++++ .../commands/conflict_user_command.go | 87 +++++++++++++++---- .../commands/conflict_user_command_test.go | 68 ++++++++++++--- 4 files changed, 190 insertions(+), 31 deletions(-) create mode 100644 pkg/cmd/grafana-cli/commands/conflict-examples/conflict_example_users.tf diff --git a/pkg/cmd/grafana-cli/commands/commands.go b/pkg/cmd/grafana-cli/commands/commands.go index 84d06624898..44e4aab888d 100644 --- a/pkg/cmd/grafana-cli/commands/commands.go +++ b/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(), }, }, diff --git a/pkg/cmd/grafana-cli/commands/conflict-examples/conflict_example_users.tf b/pkg/cmd/grafana-cli/commands/conflict-examples/conflict_example_users.tf new file mode 100644 index 00000000000..8203ea8704c --- /dev/null +++ b/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 +} diff --git a/pkg/cmd/grafana-cli/commands/conflict_user_command.go b/pkg/cmd/grafana-cli/commands/conflict_user_command.go index ed46179e85f..a1464e72e0b 100644 --- a/pkg/cmd/grafana-cli/commands/conflict_user_command.go +++ b/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: + // or + // + 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") } diff --git a/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go b/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go index 5912b6c4cf4..5dabb855245 100644 --- a/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go +++ b/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 - users []user.User - fileString string - expectedBlocks []string - expectedIdsInBlocks map[string][]string + desc string + users []user.User + fileString string + expectedValidationErr error + 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())