Several statements were missing the DefaultDatabase method #8187

corylanou · 2017-03-22T20:26:27Z

Security depends on the default database context being present for some statements. These statements were missing that check:

&influxql.AlterRetentionPolicyStatement{},
&influxql.CreateRetentionPolicyStatement{},
&influxql.CreateSubscriptionStatement{},
&influxql.DropContinuousQueryStatement{},
&influxql.DropRetentionPolicyStatement{},
&influxql.DropSubscriptionStatement{},

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

corylanou · 2017-03-23T14:55:47Z

@nathanielc let me know what you think about the test I put in place as a temporary "future proof" step. It's not ideal, but should allows us to ship this bug fix while we work on a stronger interface solution in the code base.

dgnorton · 2017-03-24T00:38:45Z

I think we also need to consider statements with a FROM clause. E.g. DELETE FROM mydb..cpu This may also require further changes on PR #1174.

nathanielc

There are 15 statements that are listed as exempt that should not be listed. See my internal comments for more details

corylanou · 2017-04-11T17:44:35Z

@nathanielc can you re-review this and let me know if you have any outstanding concerns based on the other PR's that have been filed to support this? Thanks.

nathanielc

There are still a few exempt statements that shouldn't be exempt as they specify a database.

nathanielc · 2017-04-11T18:56:14Z

influxql/ast_test.go

+		"ShowDiagnosticsStatement",
+		"ShowFieldKeysStatement",
+		"ShowGrantsForUserStatement",
+		"ShowMeasurementsStatement",


This one has a database

influxdb/influxql/ast.go

Line 2860 in b8f3b25

Database string

nathanielc · 2017-04-11T18:56:44Z

influxql/ast_test.go

+		"ShowContinuousQueriesStatement",
+		"ShowDatabasesStatement",
+		"ShowDiagnosticsStatement",
+		"ShowFieldKeysStatement",


This one too

influxdb/influxql/ast.go

Line 3281 in b8f3b25

Database string

nathanielc · 2017-04-11T18:57:33Z

influxql/ast_test.go

+		"GrantAdminStatement",
+		"KillQueryStatement",
+		"RevokeAdminStatement",
+		"SelectStatement",


I know this one is handled separately but it has a default database for sure.

Actually it has the potential to have multiple databases, so it doesn't have a DefaultDatabase, which is why it is enforced at the iterator level.

nathanielc · 2017-04-11T18:58:22Z

influxql/ast_test.go

+		"CreateDatabaseStatement",
+		"CreateUserStatement",
+		"DeleteSeriesStatement",
+		"DeleteStatement",


Doesn't this statement also allow specifying a database.

nathanielc

Found several more the have statements that are not supposed to be exempt.

nathanielc · 2017-04-12T18:00:12Z

influxql/ast_test.go

+	exemptStatements := []string{
+		"CreateDatabaseStatement",
+		"CreateUserStatement",
+		"DeleteSeriesStatement",


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

Sources Sources

This can have multiple sources, so the iterators have to handle security. DefaultDatabase would not be appropriate for this one.

nathanielc · 2017-04-12T18:00:52Z

influxql/ast_test.go

+		"DeleteSeriesStatement",
+		"DropDatabaseStatement",
+		"DropMeasurementStatement",
+		"DropSeriesStatement",


Same here

influxdb/influxql/ast.go

Line 2658 in 9e6c989

Sources Sources

Same here. If you have multiple sources, DefaultDatabase is the wrong mechanism to use.

nathanielc · 2017-04-12T18:01:47Z

influxql/ast_test.go

+		"ShowDiagnosticsStatement",
+		"ShowGrantsForUserStatement",
+		"ShowQueriesStatement",
+		"ShowRetentionPoliciesStatement",


This one has a database here

influxdb/influxql/ast.go

Line 2987 in 9e6c989

Database string

nathanielc · 2017-04-12T18:02:14Z

influxql/ast_test.go

+		"ShowGrantsForUserStatement",
+		"ShowQueriesStatement",
+		"ShowRetentionPoliciesStatement",
+		"ShowSeriesStatement",


And this one here

influxdb/influxql/ast.go

Line 2598 in 9e6c989

Database string

nathanielc · 2017-04-12T18:02:54Z

influxql/ast_test.go

+		"ShowShardsStatement",
+		"ShowStatsStatement",
+		"ShowSubscriptionsStatement",
+		"ShowTagKeysStatement",


And this one here

influxdb/influxql/ast.go

Line 3152 in 9e6c989

Database string

nathanielc · 2017-04-12T18:03:07Z

influxql/ast_test.go

+		"ShowStatsStatement",
+		"ShowSubscriptionsStatement",
+		"ShowTagKeysStatement",
+		"ShowTagValuesStatement",


And here

influxdb/influxql/ast.go

Line 3225 in 9e6c989

Database string

corylanou added the in progress label Mar 22, 2017

corylanou changed the title ~~[Create/Drop]SubscriptionStatement should satisfies HasDefaultDatabas…~~ Several statements were missing the DefaultDatabase method Mar 23, 2017

corylanou force-pushed the cjl-create-subscription-add-default-database branch 3 times, most recently from 163c577 to c74d939 Compare March 23, 2017 14:51

corylanou requested review from dgnorton and nathanielc March 23, 2017 18:57

nathanielc suggested changes Mar 29, 2017

View reviewed changes

rbetts added review and removed in progress labels Apr 2, 2017

rbetts assigned corylanou Apr 4, 2017

corylanou force-pushed the cjl-create-subscription-add-default-database branch 3 times, most recently from c704a8a to b8f3b25 Compare April 10, 2017 20:45

nathanielc suggested changes Apr 11, 2017

View reviewed changes

corylanou force-pushed the cjl-create-subscription-add-default-database branch 2 times, most recently from 83b9ff7 to 9e6c989 Compare April 11, 2017 21:28

nathanielc suggested changes Apr 12, 2017

View reviewed changes

corylanou force-pushed the cjl-create-subscription-add-default-database branch 2 times, most recently from 9462803 to d3e5861 Compare April 12, 2017 18:12

nathanielc approved these changes Apr 12, 2017

View reviewed changes

Add HasDefaultDatabase interface to several statements

9060a2a

corylanou force-pushed the cjl-create-subscription-add-default-database branch from d3e5861 to 9060a2a Compare April 12, 2017 18:41

dgnorton approved these changes Apr 12, 2017

View reviewed changes

corylanou merged commit 4ca1327 into master Apr 12, 2017

corylanou removed the review label Apr 12, 2017

rbetts added this to the 1.3.0 milestone Apr 14, 2017

rbetts added the backported/1.2 label Apr 14, 2017

rbetts mentioned this pull request Apr 26, 2017

1.1.5 merges #8323

Merged

9 tasks

mark-rushakoff deleted the cjl-create-subscription-add-default-database branch September 7, 2017 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several statements were missing the DefaultDatabase method #8187

Several statements were missing the DefaultDatabase method #8187

corylanou commented Mar 22, 2017 •

edited

Loading

corylanou commented Mar 23, 2017

dgnorton commented Mar 24, 2017

nathanielc left a comment

corylanou commented Apr 11, 2017

nathanielc left a comment

nathanielc Apr 11, 2017

nathanielc Apr 11, 2017

nathanielc Apr 11, 2017

corylanou Apr 11, 2017

nathanielc Apr 11, 2017

nathanielc left a comment •

edited

Loading

nathanielc Apr 12, 2017

corylanou Apr 12, 2017

nathanielc Apr 12, 2017

corylanou Apr 12, 2017

nathanielc Apr 12, 2017

nathanielc Apr 12, 2017

nathanielc Apr 12, 2017

nathanielc Apr 12, 2017

Several statements were missing the DefaultDatabase method #8187

Several statements were missing the DefaultDatabase method #8187

Conversation

corylanou commented Mar 22, 2017 • edited Loading

Required for all non-trivial PRs

corylanou commented Mar 23, 2017

dgnorton commented Mar 24, 2017

nathanielc left a comment

Choose a reason for hiding this comment

corylanou commented Apr 11, 2017

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corylanou commented Mar 22, 2017 •

edited

Loading

nathanielc left a comment •

edited

Loading