Account for columns on Postgres without SELECT permission #2405
Conversation
Amogh-Bharadwaj
force-pushed
the
selected-columns
branch
from
7498e77 to
6e5ed87
Compare
|
Functionally tested |
| @@ -25,10 +26,26 @@ func (c *PostgresConnector) CheckSourceTables(ctx context.Context, | |||
| var row pgx.Row | |||
| tableArr = append(tableArr, fmt.Sprintf(`(%s::text,%s::text)`, | |||
| QuoteLiteral(parsedTable.Schema), QuoteLiteral(parsedTable.Table))) | |||
|
|
|||
| selectedColumnsStr := "*" | |||
| if _, ok := excludedColumnsMap[parsedTable.Schema+"."+parsedTable.Table]; ok { | |||
There was a problem hiding this comment.
This is always going to be a pain, since we parsed req.ConnectionConfigs.TableMappings for tableNames while excludedColumnsMap is built on the raw strings from req.ConnectionConfigs.TableMappings. So you can't really use parsedTable.String() but that's what you'd want if you were going to properly handle cases like "sch.e""ma"."ta.b""le" which parses to sch.e"ma.ta.b"le
| func ConstructExcludedColumnsList(tableMappings []*protos.TableMapping) map[string][]string { | ||
| excludedColumns := make(map[string][]string) | ||
| for _, tm := range tableMappings { | ||
| excludedColumns[tm.SourceTableIdentifier] = slices.Clone(tm.Exclude) |
There was a problem hiding this comment.
| excludedColumns[tm.SourceTableIdentifier] = slices.Clone(tm.Exclude) | |
| excludedColumns[tm.SourceTableIdentifier] = tm.Exclude |
Exclude should never be mutated (I mean should as in people should not do that, code today does not do that, because doing that would be bad)
|
|
||
| // 1. Revoke all permissions on the table | ||
| if _, err := conn.Exec(context.Background(), | ||
| fmt.Sprintf("REVOKE ALL ON TABLE %s.%s FROM postgres", schemaTable.Schema, schemaTable.Table)); err != nil { |
There was a problem hiding this comment.
| fmt.Sprintf("REVOKE ALL ON TABLE %s.%s FROM postgres", schemaTable.Schema, schemaTable.Table)); err != nil { | |
| fmt.Sprintf("REVOKE ALL ON TABLE %s FROM postgres", schemaTable)); err != nil { |
should be done throughout
| dstTableName := "test_unprivileged_columns_dst" | ||
|
|
||
| _, err := s.Conn().Exec(context.Background(), fmt.Sprintf(` | ||
| CREATE TABLE IF NOT EXISTS %s ( |
There was a problem hiding this comment.
should include some oddly named columns (apostrophes etc), including in the set of excluded columns
We currently require all columns to be SELECT-able in a table for it to be synced. However, if a user is excluding columns from a table, it is a valid expectation that SELECT permissions would not be required for those excluded columns.
This PR takes a crack at supporting this use-case.
It involves:
getTableSchemaCheckSourceTablesinvalidate_mirror.goIt also adds a new E2E test and a utility function for it which grants only selected columns SELECT privileges