Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow wildcards in grants #1125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow wildcards in grants #1125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44b8e23
Allow wildcards in grants
May 17, 2022
9bd72b3
Refactor of the grant function
May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions go/logic/inspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
gosql "database/sql"
"fmt"
"reflect"
"regexp"
"strings"
"sync/atomic"
"time"
Expand All @@ -22,6 +23,35 @@ import (

const startSlavePostWaitMilliseconds = 500 * time.Millisecond

var grantOnRegexp = regexp.MustCompile(" ON `(.*?)`\\.\\*")

func grantMatch(grant string, databaseName string) bool {
matches := grantOnRegexp.FindStringSubmatch(grant)
if matches == nil {
return false
}
mysqlPattern := matches[1]
regexPattern := "^"
escape := false
for _, c := range mysqlPattern {
if escape {
regexPattern += regexp.QuoteMeta(string(c))
escape = false
} else if c == '%' {
regexPattern += ".*"
} else if c == '_' {
regexPattern += "."
} else if c == '\\' {
escape = true
} else {
regexPattern += regexp.QuoteMeta(string(c))
}
}
regexPattern += "$"
match, _ := regexp.MatchString(regexPattern, databaseName)
return match
}

// Inspector reads data from the read-MySQL-server (typically a replica, but can be the master)
// It is used for gaining initial status and structure, and later also follow up on progress and changelog
type Inspector struct {
Expand Down Expand Up @@ -242,16 +272,10 @@ func (this *Inspector) validateGrants() error {
if strings.Contains(grant, `REPLICATION SLAVE`) && strings.Contains(grant, ` ON *.*`) {
foundReplicationSlave = true
}
if strings.Contains(grant, fmt.Sprintf("GRANT ALL PRIVILEGES ON `%s`.*", this.migrationContext.DatabaseName)) {
foundDBAll = true
}
if strings.Contains(grant, fmt.Sprintf("GRANT ALL PRIVILEGES ON `%s`.*", strings.Replace(this.migrationContext.DatabaseName, "_", "\\_", -1))) {
foundDBAll = true
}
if base.StringContainsAll(grant, `ALTER`, `CREATE`, `DELETE`, `DROP`, `INDEX`, `INSERT`, `LOCK TABLES`, `SELECT`, `TRIGGER`, `UPDATE`, ` ON *.*`) {
if this.grantContainsAll(grant, "GRANT ALL PRIVILEGES ") {
foundDBAll = true
}
if base.StringContainsAll(grant, `ALTER`, `CREATE`, `DELETE`, `DROP`, `INDEX`, `INSERT`, `LOCK TABLES`, `SELECT`, `TRIGGER`, `UPDATE`, fmt.Sprintf(" ON `%s`.*", this.migrationContext.DatabaseName)) {
if this.grantContainsAll(grant, `ALTER`, `CREATE`, `DELETE`, `DROP`, `INDEX`, `INSERT`, `LOCK TABLES`, `SELECT`, `TRIGGER`, `UPDATE`) {
foundDBAll = true
}
}
Expand All @@ -278,6 +302,15 @@ func (this *Inspector) validateGrants() error {
return this.migrationContext.Log.Errorf("User has insufficient privileges for migration. Needed: SUPER|REPLICATION CLIENT, REPLICATION SLAVE and ALL on %s.*", sql.EscapeName(this.migrationContext.DatabaseName))
}

// grantContainsAll verifies the passed grant contain all the passed strings and
// that the grant match the DB
func (this *Inspector) grantContainsAll(grant string, substrings ...string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gonlo2 is it possible to add a unit test that confirms the logic in .grantContainsAll()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I could move the major part of the code to https://github.com/github/gh-ost/blob/master/go/base/utils.go as a new function, maybe GrantMatch(grant string, databaseName string) bool, and thus add some tests 🤔 . I don't know how you see it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gonlo2 I think the purpose of go/base/utils.go is for helpers that are called from many places in the code. I think it would make sense to move it there if it has many callers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timvaillancourt I've added some tests, I don't know if you have any comments now with the PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timvaillancourt Any update about this?

if !base.StringContainsAll(grant, substrings...) {
return false
}
return strings.Contains(grant, ` ON *.*`) || grantMatch(grant, this.migrationContext.DatabaseName)
}

// restartReplication is required so that we are _certain_ the binlog format and
// row image settings have actually been applied to the replication thread.
// It is entirely possible, for example, that the replication is using 'STATEMENT'
Expand Down
31 changes: 31 additions & 0 deletions go/logic/inspect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
Copyright 2022 GitHub Inc.
See https://github.com/github/gh-ost/blob/master/LICENSE
*/

package logic

import (
"testing"

"github.com/openark/golib/log"
test "github.com/openark/golib/tests"
)

func init() {
log.SetLevel(log.ERROR)
}

func TestGrantMatch(t *testing.T) {
databaseName := `my_database%name`

test.S(t).ExpectFalse(grantMatch("GRANT SELECT ON *.* TO 'user'@'%'", databaseName))
test.S(t).ExpectFalse(grantMatch("GRANT SELECT ON `other\\_database\\%name`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectTrue(grantMatch("GRANT SELECT ON `my\\_database\\%name`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectTrue(grantMatch("GRANT SELECT ON `my_database_name`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectTrue(grantMatch("GRANT SELECT ON `my\\_database\\%%`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectFalse(grantMatch("GRANT SELECT ON `database\\%%`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectTrue(grantMatch("GRANT SELECT ON `my\\_database\\%____`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectFalse(grantMatch("GRANT SELECT ON `my\\_database\\%_`.* TO 'user'@'%'", databaseName))
test.S(t).ExpectTrue(grantMatch("GRANT SELECT ON `%database%`.* TO 'user'@'%'", databaseName))
}