[part of 832]/add db id to statements #855

blakestier · 2019-07-10T22:57:41Z

🔩 Description

Adds db_id and policy_id (db_id) to iam_statements, allowing the removal of the superfluous iam_policy_statements table. Given that policy: statement is one: many, we didn't need the associations table.

I've left adding the new statement db_id as the new foreign key to iam_statement_projects as the PR's tests (and a gnarly, unfortunate rebase) made this PR unwieldy.

👍 Definition of Done

Tests pass.

👟 Demo Script / Repro Steps

Bring up hab studio, rebuild components/authz-service, CURL to create, delete, update, get policies with statements:

In ui, create new user coolbean, pw chefautomate

curl -XPOST -d '{"name":"test", "id":"newpolicy", "members":["user:local:coolbean"], "statements":[{"effect":"ALLOW", "actions":["*"] }]}' -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies --insecure

(Logged in as coolbean, with legacy polices deleted, should see Users (and be able to create/update/delete them)

curl -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies/newpolicy --insecure

curl -XPUT -d '{"name":"test", "members":["user:local:coolbean"], "statements":[{"effect":"ALLOW", "actions":["*"] }, {"effect":"DENY", "actions":["iam:users:delete"]}]}' -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies --insecure

curl -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies/newpolicy --insecure

(Coolbean should now not be able to delete users)

curl -XDELETE -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies/newpolicy --insecure

curl -H "api-token: $TOK" https://a2-dev.test/apis/iam/v2beta/policies/newpolicy --insecure (not found)

(Coolbean can't do a darn thing)

⛓️ Related Resources

add db_id as primary key to auth databases #832

✅ Checklist

Necessary tests added/updated?
Necessary docs added/updated?
Code actually executed?
Vetting performed (unit tests, lint, etc.)?

blakestier · 2019-07-13T02:55:59Z

components/authz-service/storage/postgres/datamigration/sql/01_v2_data_migrations.up.sql

@@ -1,7 +1,6 @@
 BEGIN;

 SET CONSTRAINTS iam_policy_members_policy_id_fkey DEFERRED;
-SET CONSTRAINTS iam_policy_statements_policy_id_fkey DEFERRED;


These are from the upgrade migrations-- not to be confused with those with which we cannot tamper

blakestier · 2019-07-13T02:56:57Z

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

+DROP TABLE iam_policy_statements CASCADE;
+ALTER TABLE iam_statement_projects
+    ADD UNIQUE (statement_id, project_id),
+    ADD CONSTRAINT iam_statement_projects_statement_id_fkey FOREIGN KEY (statement_id) REFERENCES iam_statements(id) ON DELETE CASCADE DEFERRABLE;


Replace constraint until db_id switch is complete to preserve cascade deletes

msorens · 2019-07-14T05:21:34Z

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

+LANGUAGE SQL;
+
+ CREATE OR REPLACE FUNCTION query_policy (_policy_id TEXT, _projects_filter TEXT[])
+    RETURNS json


I wanted to go through the exercise of seeing what is involved in converting this style of query we had been using as a team into the newer style that @srenatus started in his recent PR (#878), and also figured I could save you some grunt work, @blakestier. So I tackled query_policy and I believe I've got it working. Can chat about it when you have time.

I'd propose another round for this. Let's merge this, if we find it OK, instead of letting scope creep...

@msorens That's awesome. Once we merge this I'll give you a heads up and we can get your change in before doing any more db work! 😅

srenatus

LGTM

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

srenatus · 2019-07-15T09:23:38Z

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

+            iam_policy_statements
+        WHERE
+            statement_id = iams.id);
+DROP TABLE iam_policy_statements CASCADE;


srenatus · 2019-07-15T09:26:10Z

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

+                                COALESCE(json_agg(proj.id) FILTER (WHERE proj.id IS NOT NULL),
+                                    '[]')
+                                FROM iam_statement_projects AS stmt_projs
+                            LEFT OUTER JOIN iam_projects AS proj ON stmt_projs.statement_id = stmt.id WHERE stmt_projs.project_id = proj.id) AS projects


[nit] I believe LEFT implies OUTER:

The words INNER and OUTER are optional in all forms. INNER is the default; LEFT, RIGHT, and FULL imply an outer join.

From https://www.postgresql.org/docs/9.2/queries-table-expressions.html

Suggested change

LEFT OUTER JOIN iam_projects AS proj ON stmt_projs.statement_id = stmt.id WHERE stmt_projs.project_id = proj.id) AS projects

LEFT JOIN iam_projects AS proj ON stmt_projs.statement_id = stmt.id WHERE stmt_projs.project_id = proj.id) AS projects

But I'm not sure it matters, or if this query in its current form is what we'd want anyways. 🤔 Now that this isn't n:m, I think some of this could perhaps be further simplified...

....but OTOH this works 😄Not sure there's a simpler way at all...

We've just been explicit about joins so far . . . 🤷‍♂ I'll leave this for now in light of Michael's potential clean-up, though. I'd like to believe there's a simpler way 🦄

components/authz-service/storage/postgres/migration/sql/README.md

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

components/authz-service/storage/v2/postgres/postgres_test.go

srenatus · 2019-07-15T11:34:40Z

components/authz-service/storage/postgres/migration/sql/55_add_db_id_statements.up.sql

+LANGUAGE SQL;
+
+ CREATE OR REPLACE FUNCTION query_policy (_policy_id TEXT, _projects_filter TEXT[])
+    RETURNS json


I'd propose another round for this. Let's merge this, if we find it OK, instead of letting scope creep...

srenatus · 2019-07-15T12:16:13Z

I've left adding the new statement db_id as the new foreign key to iam_statement_projects as the PR's tests (and a gnarly, unfortunate rebase) made this PR unwieldy.

I'm on it.

srenatus · 2019-07-15T12:20:17Z

⚠️ Rebased this

Signed-off-by: Blake Johnson <bstier@chef.io>

srenatus mentioned this pull request Jul 11, 2019

[part of #832] use db_id for iam_projects and iam_[staged_]project_rules #862

Merged

4 tasks

susanev added automate-auth WIP auth-team anything that needs to be on the auth team board labels Jul 11, 2019

susanev mentioned this pull request Jul 12, 2019

add db_id as primary key to auth databases #832

Closed

19 tasks

blakestier force-pushed the blake/a2-865/add_db_id_statements branch 4 times, most recently from aecd43b to c5e5c9e Compare July 12, 2019 21:42

blakestier changed the title ~~Blake/a2 865/add db id statements~~ [A2 865]/add db id to statements Jul 12, 2019

blakestier removed the WIP label Jul 12, 2019

blakestier force-pushed the blake/a2-865/add_db_id_statements branch from c5e5c9e to 0fe7b88 Compare July 12, 2019 21:48

blakestier marked this pull request as ready for review July 13, 2019 02:27

blakestier commented Jul 13, 2019

View reviewed changes

blakestier force-pushed the blake/a2-865/add_db_id_statements branch from f20073e to 5e3a041 Compare July 13, 2019 03:02

msorens reviewed Jul 14, 2019

View reviewed changes

srenatus approved these changes Jul 15, 2019

View reviewed changes

srenatus force-pushed the blake/a2-865/add_db_id_statements branch from 5e3a041 to 47e9a01 Compare July 15, 2019 12:20

srenatus mentioned this pull request Jul 15, 2019

[part of #832] db_id for projects and policies #898

Merged

4 tasks

Blake Johnson added 3 commits July 15, 2019 08:48

Update SQL to use db_id, remove policy_statements

4a5a61b

Signed-off-by: Blake Johnson <bstier@chef.io>

Passing tests for statement/policy change

c7947dd

Signed-off-by: Blake Johnson <bstier@chef.io>

Feedback

1c7c83a

Signed-off-by: Blake Johnson <bstier@chef.io>

blakestier force-pushed the blake/a2-865/add_db_id_statements branch from 47e9a01 to 1c7c83a Compare July 15, 2019 15:49

blakestier changed the title ~~[A2 865]/add db id to statements~~ [part of 832]/add db id to statements Jul 15, 2019

msorens approved these changes Jul 15, 2019

View reviewed changes

blakestier merged commit 8fd19bf into master Jul 15, 2019

blakestier deleted the blake/a2-865/add_db_id_statements branch July 15, 2019 17:01

susanev assigned blakestier Jul 16, 2019

ekmixon mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#54

Open

snyk-bot mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#77

Open

ekmixon mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#100

Open

ekmixon mentioned this pull request Nov 28, 2023

[Snyk] Fix for 21 vulnerabilities ekmixon/automate#139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[part of 832]/add db id to statements #855

[part of 832]/add db id to statements #855

blakestier commented Jul 10, 2019 •

edited by srenatus

Loading

blakestier Jul 13, 2019

blakestier Jul 13, 2019

msorens Jul 14, 2019

srenatus Jul 15, 2019

blakestier Jul 15, 2019

srenatus left a comment

srenatus Jul 15, 2019

srenatus Jul 15, 2019

srenatus Jul 15, 2019

blakestier Jul 15, 2019

srenatus Jul 15, 2019

srenatus commented Jul 15, 2019

srenatus commented Jul 15, 2019

	LEFT OUTER JOIN iam_projects AS proj ON stmt_projs.statement_id = stmt.id WHERE stmt_projs.project_id = proj.id) AS projects
	LEFT JOIN iam_projects AS proj ON stmt_projs.statement_id = stmt.id WHERE stmt_projs.project_id = proj.id) AS projects

[part of 832]/add db id to statements #855

[part of 832]/add db id to statements #855

Conversation

blakestier commented Jul 10, 2019 • edited by srenatus Loading

🔩 Description

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

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

srenatus 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

srenatus commented Jul 15, 2019

srenatus commented Jul 15, 2019

blakestier commented Jul 10, 2019 •

edited by srenatus

Loading