Create versioned views WITH (security_invoker = true)
#189
Conversation
pkg/roll/roll.go
Outdated
| pgConn: conn, | ||
| schema: schema, | ||
| state: state, | ||
| viewCreator: viewCreator, |
There was a problem hiding this comment.
not against this but I guess that we will do conditional statements based on the version in many places. Perhaps it would be good to store the version as a field here and read it from the createViews function to use one creation mode or another
There was a problem hiding this comment.
I created the interface and two implementations because I think it's cleaner to do this kind of version switching at the top-level with dependency injection rather than having switches deeper in the code.
It's not yet clear to me where/how many such switches we will have though.
I've rebased to store the postgres version and use it as a switch in createView, which is less new code.
LMK which approach you prefer.
.github/workflows/build.yml
Outdated
| run: go test ./... | ||
| env: | ||
| POSTGRES_VERSION: ${{ matrix.pgVersion }} | ||
|
|
||
| - name: Run pg14 tests | ||
| if: matrix.pgVersion == '14.8' | ||
| run: go test ./... --skip 'TestViewsAreCreatedWithSecurityInvokerTrue' |
There was a problem hiding this comment.
Could we skip the test from the test case instead? calling t.Skip or similar based on the postgres version
There was a problem hiding this comment.
I've rebased to skip the test as you suggest rather than skipping in github actions.
andrew-farries
force-pushed
the
security-invoker-views
branch
from
d695ada
to
adc49d7
Compare
There was a problem hiding this comment.
LGTM
My impression is that as we make progress (and new Postgres releases appear), we will do this more and more. Over the time more features will appear making zero downtime changes easier, or that I would hope
Having that into account, this approach looks good to me. As of today any option would work for me, my only worry is that if we solve this through dependency injection the Roll struct will only grow on logic that could be more local
| columns := make([]string, 0, len(table.Columns)) | ||
| for k, v := range table.Columns { | ||
| columns = append(columns, fmt.Sprintf("%s AS %s", pq.QuoteIdentifier(v.Name), pq.QuoteIdentifier(k))) | ||
| } | ||
|
|
||
| withOptions := "" |
There was a problem hiding this comment.
Could you add a comment here to explain security_invoker? 🙏
andrew-farries
force-pushed
the
security-invoker-views
branch
from
adc49d7
to
4a3ceee
Compare
Ensure that views (in Postgres 15 and 16) are created with
security_invoker = true.This ensures that any row-level security on the underlying table is applied according to the permissions of the invoker of the view rather than its owner.
Postgres 14 does not support the creation of views with
security_invoker = true.See:
Closes #179