Skip to content

Commit fd64472

Browse files
bplunkett-stripebplunkett-stripe
committed
Table priv support
1 parent 5df33b7 commit fd64472

File tree

12 files changed

+707
-7
lines changed

12 files changed

+707
-7
lines changed

‎README.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,6 @@ Unsupported: <= 13 are not supported. Use at your own risk.
204204

205205
# Unsupported migrations
206206
An abridged list of unsupported migrations:
207-
- Privileges (Planned)
208207
- Types (Only enums are currently supported)
209208
- Renaming. The diffing library relies on names to identify the old and new versions of a table, index, etc. If you rename
210209
an object, it will be treated as a drop and an add

‎cmd/pg-schema-diff/apply_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func runPlan(ctx context.Context, cmd *cobra.Command, connConfig *pgx.ConnConfig
150150
return fmt.Errorf("setting lock timeout: %w", err)
151151
}
152152
if _, err := conn.ExecContext(ctx, stmt.ToSQL()); err != nil {
153-
return fmt.Errorf("executing migration statement. the database maybe be in a dirty state: %s: %w", stmt, err)
153+
return fmt.Errorf("executing migration statement. the database maybe be in a dirty state: %s: %w", stmt.DDL, err)
154154
}
155155
cmdPrintf(cmd, "Finished executing statement. Duration: %s\n", time.Since(start))
156156
}

‎internal/migration_acceptance_tests/privilege_cases_test.go‎

Lines changed: 330 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,330 @@
1+
package migration_acceptance_tests
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stripe/pg-schema-diff/pkg/diff"
7+
)
8+
9+
var privilegeAcceptanceTestCases = []acceptanceTestCase{
10+
{
11+
name: "no-op",
12+
roles: []string{
13+
"app_user",
14+
},
15+
oldSchemaDDL: []string{
16+
`
17+
CREATE TABLE foobar(id INT);
18+
GRANT SELECT ON foobar TO app_user;
19+
`,
20+
},
21+
newSchemaDDL: []string{
22+
`
23+
CREATE TABLE foobar(id INT);
24+
GRANT SELECT ON foobar TO app_user;
25+
`,
26+
},
27+
expectEmptyPlan: true,
28+
},
29+
{
30+
name: "Grant SELECT to role",
31+
roles: []string{"app_user"},
32+
oldSchemaDDL: []string{
33+
`CREATE TABLE foobar(id INT);`,
34+
},
35+
newSchemaDDL: []string{
36+
`
37+
CREATE TABLE foobar(id INT);
38+
GRANT SELECT ON foobar TO app_user;
39+
`,
40+
},
41+
expectedHazardTypes: []diff.MigrationHazardType{
42+
diff.MigrationHazardTypeAuthzUpdate,
43+
},
44+
},
45+
{
46+
name: "Grant multiple privileges to role",
47+
roles: []string{"app_user"},
48+
oldSchemaDDL: []string{
49+
`CREATE TABLE foobar(id INT);`,
50+
},
51+
newSchemaDDL: []string{
52+
`
53+
CREATE TABLE foobar(id INT);
54+
GRANT SELECT, INSERT, UPDATE, DELETE ON foobar TO app_user;
55+
`,
56+
},
57+
expectedHazardTypes: []diff.MigrationHazardType{
58+
diff.MigrationHazardTypeAuthzUpdate,
59+
},
60+
},
61+
{
62+
name: "Grant to PUBLIC",
63+
oldSchemaDDL: []string{
64+
`CREATE TABLE foobar(id INT);`,
65+
},
66+
newSchemaDDL: []string{
67+
`
68+
CREATE TABLE foobar(id INT);
69+
GRANT SELECT ON foobar TO PUBLIC;
70+
`,
71+
},
72+
expectedHazardTypes: []diff.MigrationHazardType{
73+
diff.MigrationHazardTypeAuthzUpdate,
74+
},
75+
},
76+
{
77+
name: "Revoke privilege from role",
78+
roles: []string{"app_user"},
79+
oldSchemaDDL: []string{
80+
`
81+
CREATE TABLE foobar(id INT);
82+
GRANT SELECT ON foobar TO app_user;
83+
`,
84+
},
85+
newSchemaDDL: []string{
86+
`CREATE TABLE foobar(id INT);`,
87+
},
88+
expectedHazardTypes: []diff.MigrationHazardType{
89+
diff.MigrationHazardTypeAuthzUpdate,
90+
},
91+
},
92+
{
93+
name: "Grant WITH GRANT OPTION",
94+
roles: []string{"app_user"},
95+
oldSchemaDDL: []string{
96+
`CREATE TABLE foobar(id INT);`,
97+
},
98+
newSchemaDDL: []string{
99+
`
100+
CREATE TABLE foobar(id INT);
101+
GRANT SELECT ON foobar TO app_user WITH GRANT OPTION;
102+
`,
103+
},
104+
expectedHazardTypes: []diff.MigrationHazardType{
105+
diff.MigrationHazardTypeAuthzUpdate,
106+
},
107+
},
108+
{
109+
name: "Change GRANT OPTION (recreates privilege)",
110+
roles: []string{"app_user"},
111+
oldSchemaDDL: []string{
112+
`
113+
CREATE TABLE foobar(id INT);
114+
GRANT SELECT ON foobar TO app_user;
115+
`,
116+
},
117+
newSchemaDDL: []string{
118+
`
119+
CREATE TABLE foobar(id INT);
120+
GRANT SELECT ON foobar TO app_user WITH GRANT OPTION;
121+
`,
122+
},
123+
expectedHazardTypes: []diff.MigrationHazardType{
124+
diff.MigrationHazardTypeAuthzUpdate,
125+
},
126+
},
127+
{
128+
name: "Remove GRANT OPTION (recreates privilege)",
129+
roles: []string{"app_user"},
130+
oldSchemaDDL: []string{
131+
`
132+
CREATE TABLE foobar(id INT);
133+
GRANT SELECT ON foobar TO app_user WITH GRANT OPTION;
134+
`,
135+
},
136+
newSchemaDDL: []string{
137+
`
138+
CREATE TABLE foobar(id INT);
139+
GRANT SELECT ON foobar TO app_user;
140+
`,
141+
},
142+
expectedHazardTypes: []diff.MigrationHazardType{
143+
diff.MigrationHazardTypeAuthzUpdate,
144+
},
145+
},
146+
{
147+
name: "Grant on new table (no hazards since table is new)",
148+
roles: []string{"app_user"},
149+
newSchemaDDL: []string{
150+
`
151+
CREATE TABLE foobar(id INT);
152+
GRANT SELECT ON foobar TO app_user;
153+
`,
154+
},
155+
// No hazards expected since table is brand new
156+
},
157+
{
158+
name: "Drop table with privileges (only DeletesData hazard)",
159+
roles: []string{"app_user"},
160+
oldSchemaDDL: []string{
161+
`
162+
CREATE TABLE foobar(id INT);
163+
GRANT SELECT ON foobar TO app_user;
164+
`,
165+
},
166+
expectedHazardTypes: []diff.MigrationHazardType{
167+
diff.MigrationHazardTypeDeletesData,
168+
},
169+
},
170+
{
171+
name: "Grant on non-public schema table",
172+
roles: []string{"app_user"},
173+
oldSchemaDDL: []string{
174+
`
175+
CREATE SCHEMA app_schema;
176+
CREATE TABLE app_schema.foobar(id INT);
177+
`,
178+
},
179+
newSchemaDDL: []string{
180+
`
181+
CREATE SCHEMA app_schema;
182+
CREATE TABLE app_schema.foobar(id INT);
183+
GRANT SELECT ON app_schema.foobar TO app_user;
184+
`,
185+
},
186+
expectedHazardTypes: []diff.MigrationHazardType{
187+
diff.MigrationHazardTypeAuthzUpdate,
188+
},
189+
},
190+
{
191+
name: "Multiple roles with different privileges",
192+
roles: []string{"reader", "writer"},
193+
oldSchemaDDL: []string{
194+
`CREATE TABLE foobar(id INT);`,
195+
},
196+
newSchemaDDL: []string{
197+
`
198+
CREATE TABLE foobar(id INT);
199+
GRANT SELECT ON foobar TO reader;
200+
GRANT SELECT, INSERT, UPDATE, DELETE ON foobar TO writer;
201+
`,
202+
},
203+
expectedHazardTypes: []diff.MigrationHazardType{
204+
diff.MigrationHazardTypeAuthzUpdate,
205+
},
206+
},
207+
{
208+
name: "Privileges preserved through unrelated table changes (add column)",
209+
roles: []string{"app_user"},
210+
oldSchemaDDL: []string{
211+
`
212+
CREATE TABLE foobar(id INT);
213+
GRANT SELECT ON foobar TO app_user;
214+
`,
215+
},
216+
newSchemaDDL: []string{
217+
`
218+
CREATE TABLE foobar(id INT, new_col TEXT);
219+
GRANT SELECT ON foobar TO app_user;
220+
`,
221+
},
222+
// No privilege hazards expected - only the column change
223+
},
224+
{
225+
name: "Revoke and grant different privilege",
226+
roles: []string{"app_user"},
227+
oldSchemaDDL: []string{
228+
`
229+
CREATE TABLE foobar(id INT);
230+
GRANT SELECT ON foobar TO app_user;
231+
`,
232+
},
233+
newSchemaDDL: []string{
234+
`
235+
CREATE TABLE foobar(id INT);
236+
GRANT INSERT ON foobar TO app_user;
237+
`,
238+
},
239+
expectedHazardTypes: []diff.MigrationHazardType{
240+
diff.MigrationHazardTypeAuthzUpdate,
241+
},
242+
},
243+
{
244+
name: "Grant all privilege types",
245+
roles: []string{"app_user"},
246+
oldSchemaDDL: []string{
247+
`CREATE TABLE foobar(id INT);`,
248+
},
249+
newSchemaDDL: []string{
250+
`
251+
CREATE TABLE foobar(id INT);
252+
GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER ON foobar TO app_user;
253+
`,
254+
},
255+
expectedHazardTypes: []diff.MigrationHazardType{
256+
diff.MigrationHazardTypeAuthzUpdate,
257+
},
258+
},
259+
{
260+
name: "Grant on partitioned parent table",
261+
roles: []string{"app_user"},
262+
oldSchemaDDL: []string{
263+
`
264+
CREATE TABLE foobar(
265+
category TEXT
266+
) partition by list (category);
267+
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category_1');
268+
`,
269+
},
270+
newSchemaDDL: []string{
271+
`
272+
CREATE TABLE foobar(
273+
category TEXT
274+
) partition by list (category);
275+
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category_1');
276+
GRANT SELECT ON foobar TO app_user;
277+
`,
278+
},
279+
expectedHazardTypes: []diff.MigrationHazardType{
280+
diff.MigrationHazardTypeAuthzUpdate,
281+
},
282+
},
283+
{
284+
name: "Privilege on new partition (not implemented)",
285+
roles: []string{"app_user"},
286+
oldSchemaDDL: []string{
287+
`
288+
CREATE TABLE foobar(
289+
category TEXT
290+
) partition by list (category);
291+
`,
292+
},
293+
newSchemaDDL: []string{
294+
`
295+
CREATE TABLE foobar(
296+
category TEXT
297+
) partition by list (category);
298+
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category');
299+
GRANT SELECT ON foobar_1 TO app_user;
300+
`,
301+
},
302+
expectedPlanErrorIs: diff.ErrNotImplemented,
303+
},
304+
{
305+
name: "Add privilege on existing partition (not implemented)",
306+
roles: []string{"app_user"},
307+
oldSchemaDDL: []string{
308+
`
309+
CREATE TABLE foobar(
310+
category TEXT
311+
) partition by list (category);
312+
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category');
313+
`,
314+
},
315+
newSchemaDDL: []string{
316+
`
317+
CREATE TABLE foobar(
318+
category TEXT
319+
) partition by list (category);
320+
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category');
321+
GRANT SELECT ON foobar_1 TO app_user;
322+
`,
323+
},
324+
expectedPlanErrorIs: diff.ErrNotImplemented,
325+
},
326+
}
327+
328+
func TestPrivilegeCases(t *testing.T) {
329+
runTestCases(t, privilegeAcceptanceTestCases)
330+
}

‎internal/queries/queries.sql‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -624,3 +624,45 @@ WHERE
624624
AND depend.objid = c.oid
625625
AND depend.deptype = 'e'
626626
);
627+
628+
-- name: GetTablePrivileges :many
629+
WITH parsed_acl AS (
630+
SELECT
631+
c.oid AS table_oid,
632+
c.relname AS table_name,
633+
n.nspname AS table_schema_name,
634+
c.relowner AS owner_oid,
635+
(ACLEXPLODE(c.relacl)).grantee AS grantee_oid,
636+
(ACLEXPLODE(c.relacl)).privilege_type AS privilege_type,
637+
(ACLEXPLODE(c.relacl)).is_grantable AS is_grantable
638+
FROM pg_catalog.pg_class AS c
639+
INNER JOIN pg_catalog.pg_namespace AS n ON c.relnamespace = n.oid
640+
WHERE
641+
n.nspname NOT IN ('pg_catalog', 'information_schema')
642+
AND n.nspname !~ '^pg_toast'
643+
AND n.nspname !~ '^pg_temp'
644+
AND (c.relkind = 'r' OR c.relkind = 'p')
645+
AND c.relacl IS NOT null
646+
-- Exclude tables owned by extensions
647+
AND NOT EXISTS (
648+
SELECT depend.objid
649+
FROM pg_catalog.pg_depend AS depend
650+
WHERE
651+
depend.classid = 'pg_class'::REGCLASS
652+
AND depend.objid = c.oid
653+
AND depend.deptype = 'e'
654+
)
655+
)
656+
657+
SELECT
658+
pa.table_name::TEXT,
659+
pa.table_schema_name::TEXT,
660+
COALESCE(grantee_role.rolname, '')::TEXT AS grantee,
661+
pa.privilege_type::TEXT AS privilege,
662+
pa.is_grantable
663+
FROM parsed_acl AS pa
664+
LEFT JOIN pg_catalog.pg_roles AS grantee_role
665+
ON pa.grantee_oid = grantee_role.oid
666+
-- Exclude privileges granted to the table owner (these are implicit)
667+
WHERE pa.grantee_oid != pa.owner_oid OR pa.grantee_oid = 0
668+
ORDER BY pa.table_schema_name, pa.table_name, grantee, pa.privilege_type;

0 commit comments

Comments
 (0)