Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
6 changes: 3 additions & 3 deletions encryptor/searchable_query_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (filter *SearchableQueryFilter) filterInterestingTables(fromExp sqlparser.T
// And even then, we can work only with tables that we have an encryption schema for.
var encryptableTables []*AliasedTableName
for _, table := range tables {
if v := filter.schemaStore.GetTableSchema(table.TableName.Name.String()); v != nil {
if v := filter.schemaStore.GetTableSchema(table.TableName.Name.ValueForConfig()); v != nil {
encryptableTables = append(encryptableTables, table)
}
}
Expand Down Expand Up @@ -174,9 +174,9 @@ func (filter *SearchableQueryFilter) filterComparisons(exprs []*sqlparser.Compar

func (filter *SearchableQueryFilter) getTableSchemaOfColumn(column *sqlparser.ColName, defaultTable *AliasedTableName, aliasedTables AliasToTableMap) config.TableSchema {
if column.Qualifier.Qualifier.IsEmpty() {
return filter.schemaStore.GetTableSchema(defaultTable.TableName.Name.String())
return filter.schemaStore.GetTableSchema(defaultTable.TableName.Name.ValueForConfig())
}
tableName := aliasedTables[column.Qualifier.Name.String()]
tableName := aliasedTables[column.Qualifier.Name.ValueForConfig()]
return filter.schemaStore.GetTableSchema(tableName)
}

Expand Down
74 changes: 74 additions & 0 deletions encryptor/searchable_query_filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package encryptor

import (
"fmt"
"testing"

"github.com/cossacklabs/acra/encryptor/config"
"github.com/cossacklabs/acra/sqlparser"
)

func TestGetTableSchemaOfColumnMatchConfiTable(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func TestGetTableSchemaOfColumnMatchConfiTable(t *testing.T) {
func TestGetTableSchemaOfColumnMatchConfigTable(t *testing.T) {

tableName := "some_table"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use name with upper-cased characters here and below? to be sure that we handle it correctly. Because previously we handled it in lowercase by default

configStr := fmt.Sprintf(`
schemas:
- table: %s
encrypted:
- column: "default_client_id"
- column: specified_client_id
client_id: specified_client_id
`, tableName)
schemaStore, err := config.MapTableSchemaStoreFromConfig([]byte(configStr))
if err != nil {
t.Fatalf("Can't parse config: %s", err.Error())
}

searchableQueryFilter := SearchableQueryFilter{
schemaStore: schemaStore,
}

tableNamesWithQuotes := sqlparser.NewTableIdentWithQuotes(tableName, '"')
schemaTable := searchableQueryFilter.getTableSchemaOfColumn(&sqlparser.ColName{}, &AliasedTableName{
TableName: sqlparser.TableName{
Name: tableNamesWithQuotes,
},
}, AliasToTableMap{})

if schemaTable == nil {
t.Fatalf("Expect not nil schemaTable, matched with config")
}
}

func TestFilterInterestingTables(t *testing.T) {
tableName := "some_table"
configStr := fmt.Sprintf(`
schemas:
- table: %s
encrypted:
- column: "default_client_id"
- column: specified_client_id
client_id: specified_client_id
`, tableName)
schemaStore, err := config.MapTableSchemaStoreFromConfig([]byte(configStr))
if err != nil {
t.Fatalf("Can't parse config: %s", err.Error())
}

searchableQueryFilter := SearchableQueryFilter{
schemaStore: schemaStore,
}

tableNamesWithQuotes := sqlparser.NewTableIdentWithQuotes(tableName, '"')

aliasedTable, _ := searchableQueryFilter.filterInterestingTables(sqlparser.TableExprs{
&sqlparser.AliasedTableExpr{
Expr: sqlparser.TableName{
Name: tableNamesWithQuotes,
},
},
})

if aliasedTable == nil {
t.Fatalf("Expect not nil aliasedTable, matched with config")
}
}
2 changes: 1 addition & 1 deletion encryptor/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ func mapColumnsToAliases(selectQuery *sqlparser.Select) ([]*columnInfo, error) {
if ok {
if len(joinTables) > 0 {
if !starExpr.TableName.Name.IsEmpty() {
joinTable, ok := joinAliases[starExpr.TableName.Name.String()]
joinTable, ok := joinAliases[starExpr.TableName.Name.ValueForConfig()]
if !ok {
return nil, errUnsupportedExpression
}
Expand Down
30 changes: 30 additions & 0 deletions sqlparser/ast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/cossacklabs/acra/sqlparser/dialect/mysql"
"github.com/cossacklabs/acra/sqlparser/dialect/postgresql"
"reflect"
"strconv"
"strings"
"testing"
"unsafe"
Expand Down Expand Up @@ -535,6 +536,35 @@ func TestTableIdentMarshal(t *testing.T) {
}
}

func TestTableIdentValueForConfig(t *testing.T) {
str := TableIdent{
quote: 34,
v: "table",
}
b, err := json.Marshal(str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we use json package instead of sqlparser.String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just copied from another test.

if err != nil {
t.Fatal(err)
}
got := string(b)
want := `"table"`
if got != want {
t.Errorf("json.Marshal()= %s, want %s", got, want)
}
tableForConfig := str.ValueForConfig()
if tableForConfig == got {
t.Errorf("ValueForConfig should not be equal with init %s, want %s", got, want)
}

unquoted, err := strconv.Unquote(got)
if err != nil {
t.Fatal(err)
}

if tableForConfig != unquoted {
t.Errorf("ValueForConfig should be equal with unquoted value %s, want %s", got, want)
}
}

func TestHexDecode(t *testing.T) {
testcase := []struct {
in, out string
Expand Down
38 changes: 37 additions & 1 deletion sqlparser/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,33 @@ var (
output: "select /* string table alias */ 1 from t as 't1'",
// mysql allow to use single quote for column/table aliases
dialect: mysql.NewMySQLDialect(),
}, {
input: `select * from mytable where "AGE" = 1 and "TEST" = 'test'`,
// postgres allow to use double quote string for columns
dialect: postgresql.NewPostgreSQLDialect(),
}, {
// this is valid query ONLY for MySQL in default mode, for now,
// but invalid for PostgreSQL and MySQL in ANSI mode and maybe be changed in future
input: `insert into some_table(id, data) VALUES (10918, "test")`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets add comment, that it is temporary valid for MySQL default settings, but invalid for PostgreSQL & MySQL with ANSI mode, and probably will be changed. It will help in the future to understand why this case will fail after future changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, also added TODO in invalidSQL tests that such query should be tests on failure

output: `insert into some_table(id, data) values (10918, 'test')`,
}, {
input: `insert into some_table(id, data) values (10918, 'test')`,
}, {
input: `select * from mytable where "test" = "test"`,
output: `select * from mytable where 'test' = 'test'`,
}, {
input: `select * from mytable where "test" = 1 and 'value' = 'value'`,
output: `select * from mytable where 'test' = 1 and 'value' = 'value'`,
}, {
input: `SELECT "id", "landline_number" AS "landlineNumber", "removal" FROM "users" AS "User" where "User"."is_active"`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets add some somparison too: WHERE "User"."AGE" = 123 and simple "AGE"='123' without table name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

plus I remember that you mentioned some integration tests with double quotes. Can we extend them with additional cases that we caught?

output: `select "id", "landline_number" as "landlineNumber", "removal" from "users" as "User" where "User"."is_active"`,
dialect: postgresql.NewPostgreSQLDialect(),
}, {
input: `select "id" from "users" as "User" where "User"."AGE" = 123`,
dialect: postgresql.NewPostgreSQLDialect(),
}, {
input: `select "id" from "users" as "User" where "AGE" = '123'`,
dialect: postgresql.NewPostgreSQLDialect(),
}, {
input: "select /* string table alias without as */ 1 from t 't1'",
output: "select /* string table alias without as */ 1 from t as 't1'",
Expand Down Expand Up @@ -1322,11 +1349,17 @@ var (
)

func TestValid(t *testing.T) {
var testDialect dialect.Dialect
for i, tcase := range validSQL {
if tcase.output == "" {
tcase.output = tcase.input
}
tree, err := New(ModeStrict).Parse(tcase.input)

testDialect = tcase.dialect
if tcase.dialect == nil {
testDialect = mysql.NewMySQLDialect()
}
tree, err := ParseWithDialect(testDialect, tcase.input)
if err != nil {
t.Errorf("Parse(%q) err: %v, want nil", tcase.input, err)
continue
Expand Down Expand Up @@ -1656,6 +1689,9 @@ func TestConvert(t *testing.T) {
input: "select convert('abc', decimal(4+9)) from t",
output: "syntax error at position 33",
},
// TODO: added test cases to cover errors for MySQL ANSI mode
// `insert into table (id, name) values (125, "data")` currently, in ANSI mod its valid query with contains
// Rows {SQLVal(125), ColName("data")} - but it should fail with error
}

var dialect dialect.Dialect
Expand Down
Loading