-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
Several statements were missing the DefaultDatabase method #8187
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,6 +2,7 @@ package influxql_test | |||
|
||||
import ( | ||||
"fmt" | ||||
"go/importer" | ||||
"reflect" | ||||
"strings" | ||||
"testing" | ||||
|
@@ -1700,6 +1701,102 @@ func TestParse_Errors(t *testing.T) { | |||
} | ||||
} | ||||
|
||||
// This test checks to ensure that we have given thought to the database | ||||
// context required for security checks. If a new statement is added, this | ||||
// test will fail until it is categorized into the correct bucket below. | ||||
func Test_EnforceHasDefaultDatabase(t *testing.T) { | ||||
pkg, err := importer.Default().Import("github.com/influxdata/influxdb/influxql") | ||||
if err != nil { | ||||
fmt.Printf("error: %s\n", err.Error()) | ||||
return | ||||
} | ||||
statements := []string{} | ||||
|
||||
// this is a list of statements that do not have a database context | ||||
exemptStatements := []string{ | ||||
"CreateDatabaseStatement", | ||||
"CreateUserStatement", | ||||
"DeleteSeriesStatement", | ||||
"DropDatabaseStatement", | ||||
"DropMeasurementStatement", | ||||
"DropSeriesStatement", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here Line 2658 in 9e6c989
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. If you have multiple sources, DefaultDatabase is the wrong mechanism to use. |
||||
"DropShardStatement", | ||||
"DropUserStatement", | ||||
"GrantAdminStatement", | ||||
"KillQueryStatement", | ||||
"RevokeAdminStatement", | ||||
"SelectStatement", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this one is handled separately but it has a default database for sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually it has the potential to have multiple databases, so it doesn't have a |
||||
"SetPasswordUserStatement", | ||||
"ShowContinuousQueriesStatement", | ||||
"ShowDatabasesStatement", | ||||
"ShowDiagnosticsStatement", | ||||
"ShowGrantsForUserStatement", | ||||
"ShowQueriesStatement", | ||||
"ShowShardGroupsStatement", | ||||
"ShowShardsStatement", | ||||
"ShowStatsStatement", | ||||
"ShowSubscriptionsStatement", | ||||
"ShowUsersStatement", | ||||
} | ||||
|
||||
exists := func(stmt string) bool { | ||||
switch stmt { | ||||
// These are functions with the word statement in them, and can be ignored | ||||
case "Statement", "MustParseStatement", "ParseStatement", "RewriteStatement": | ||||
return true | ||||
default: | ||||
// check the exempt statements | ||||
for _, s := range exemptStatements { | ||||
if s == stmt { | ||||
return true | ||||
} | ||||
} | ||||
// check the statements that passed the interface test for HasDefaultDatabase | ||||
for _, s := range statements { | ||||
if s == stmt { | ||||
return true | ||||
} | ||||
} | ||||
return false | ||||
} | ||||
} | ||||
|
||||
needsHasDefault := []interface{}{ | ||||
&influxql.AlterRetentionPolicyStatement{}, | ||||
&influxql.CreateContinuousQueryStatement{}, | ||||
&influxql.CreateRetentionPolicyStatement{}, | ||||
&influxql.CreateSubscriptionStatement{}, | ||||
&influxql.DeleteStatement{}, | ||||
&influxql.DropContinuousQueryStatement{}, | ||||
&influxql.DropRetentionPolicyStatement{}, | ||||
&influxql.DropSubscriptionStatement{}, | ||||
&influxql.GrantStatement{}, | ||||
&influxql.RevokeStatement{}, | ||||
&influxql.ShowFieldKeysStatement{}, | ||||
&influxql.ShowMeasurementsStatement{}, | ||||
&influxql.ShowRetentionPoliciesStatement{}, | ||||
&influxql.ShowSeriesStatement{}, | ||||
&influxql.ShowTagKeysStatement{}, | ||||
&influxql.ShowTagValuesStatement{}, | ||||
} | ||||
|
||||
for _, stmt := range needsHasDefault { | ||||
statements = append(statements, strings.TrimPrefix(fmt.Sprintf("%T", stmt), "*influxql.")) | ||||
if _, ok := stmt.(influxql.HasDefaultDatabase); !ok { | ||||
t.Errorf("%T was expected to declare DefaultDatabase method", stmt) | ||||
} | ||||
|
||||
} | ||||
|
||||
for _, declName := range pkg.Scope().Names() { | ||||
if strings.HasSuffix(declName, "Statement") { | ||||
if !exists(declName) { | ||||
t.Errorf("unchecked statement %s. please update this test to determine if this statement needs to declare 'DefaultDatabase'", declName) | ||||
} | ||||
} | ||||
} | ||||
} | ||||
|
||||
// Valuer represents a simple wrapper around a map to implement the influxql.Valuer interface. | ||||
type Valuer map[string]interface{} | ||||
|
||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this one can have a database. There don't appear to be any docs on DeleteSeries so not 100% sure but the type has a source
influxdb/influxql/ast.go
Line 2689 in 9e6c989
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can have multiple sources, so the iterators have to handle security. DefaultDatabase would not be appropriate for this one.