Skip to content

Commit

Permalink
parser: validate privilege name when parsing
Browse files Browse the repository at this point in the history
Now we differentiate between parsing a privileges name from user input
versus from a system table. This allows us to validate names, but still
backport names to older branches without causing errors.

No release note, since the user-facing change here is just a better
error message.

Release note: None
  • Loading branch information
rafiss committed Aug 28, 2023
1 parent e6e624c commit 5ecea79
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 9 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/parser/sql.y
Original file line number Diff line number Diff line change
Expand Up @@ -6279,7 +6279,7 @@ privileges:
}
| privilege_list
{
privList, err := privilege.ListFromStrings($1.nameList().ToStrings())
privList, err := privilege.ListFromStrings($1.nameList().ToStrings(), privilege.OriginFromUserInput)
if err != nil {
return setErr(sqllex, err)
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/sql/parser/testdata/alter_default_privileges
Original file line number Diff line number Diff line change
Expand Up @@ -942,3 +942,11 @@ ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM foo, bar --
ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM foo, bar -- fully parenthesized
ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM foo, bar -- literals removed
ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM _, _ -- identifiers removed

error
ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT CREATE, UNKNOWN_PRIV ON TYPES TO SESSION_USER WITH GRANT OPTION
----
at or near "on": syntax error: not a valid privilege: "unknown_priv"
DETAIL: source SQL:
ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT CREATE, UNKNOWN_PRIV ON TYPES TO SESSION_USER WITH GRANT OPTION
^
8 changes: 8 additions & 0 deletions pkg/sql/parser/testdata/grant_revoke
Original file line number Diff line number Diff line change
Expand Up @@ -596,3 +596,11 @@ DETAIL: source SQL:
REVOKE SELECT ON ROLE foo, bar FROM blix
^
HINT: try \h REVOKE

error
GRANT CREATE, UNKNOWN_PRIV ON TABLE foo TO testuser
----
at or near "on": syntax error: not a valid privilege: "unknown_priv"
DETAIL: source SQL:
GRANT CREATE, UNKNOWN_PRIV ON TABLE foo TO testuser
^
32 changes: 26 additions & 6 deletions pkg/sql/privilege/privilege.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,18 +336,38 @@ func PrivilegesFromBitFields(
return ret, nil
}

// Origin indicates the origin of the privileges being parsed in
// ListFromStrings.
type Origin bool

const (
// OriginFromUserInput indicates that the privilege name came from user
// input and should be validated to make sure it refers to a real privilege.
OriginFromUserInput Origin = false

// OriginFromSystemTable indicates that the privilege name came from a
// system table and should be ignored if it does not refer to a real
// privilege.
OriginFromSystemTable Origin = true
)

// ListFromStrings takes a list of strings and attempts to build a list of Kind.
// We convert each string to uppercase and search for it in the ByName map.
// If an entry is not found in ByName, it is ignored.
func ListFromStrings(strs []string) (List, error) {
// If an entry is not found in ByName, it is either ignored or reports an error
// depending on the purpose.
func ListFromStrings(strs []string, origin Origin) (List, error) {
ret := make(List, len(strs))
for i, s := range strs {
k, ok := ByName[strings.ToUpper(s)]
if !ok {
// Ignore an unknown privilege name. This is so that it is possible to
// backport new privileges onto older release branches, without causing
// mixed-version compatibility issues.
continue
// Ignore an unknown privilege name if it came from a system table. This
// is so that it is possible to backport new privileges onto older release
// branches, without causing mixed-version compatibility issues.
if origin == OriginFromSystemTable {
continue
} else if origin == OriginFromUserInput {
return nil, errors.Errorf("not a valid privilege: %q", s)
}
}
ret[i] = k
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/syntheticprivilegecache/accumulator.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ func (s *accumulator) addRow(path, user tree.DString, privArr, grantOptionArr *t
for _, elem := range grantOptionArr.Array {
grantOptionStrings = append(grantOptionStrings, string(tree.MustBeDString(elem)))
}
privs, err := privilege.ListFromStrings(privilegeStrings)
privs, err := privilege.ListFromStrings(privilegeStrings, privilege.OriginFromSystemTable)
if err != nil {
return err
}
grantOptions, err := privilege.ListFromStrings(grantOptionStrings)
grantOptions, err := privilege.ListFromStrings(grantOptionStrings, privilege.OriginFromSystemTable)
if err != nil {
return err
}
Expand Down

0 comments on commit 5ecea79

Please sign in to comment.