Grant owner role to connected user for default privileges management #71

zytek · 2019-04-02T09:42:40Z

Make sure connected user has proper permissions to manage default
privileges.

Fixed #70

NOTE: please assist in running acc tests, I have tested it only with my dev RDS

zytek · 2019-04-02T09:48:59Z

Usage example possible now: https://gist.github.com/zytek/bd87ae7f52d25dc0743226a63d07c5d4

cyrilgdn

@zytek Many thanks for your work on this issue and sorry for the response delay.

I added a few remarks.

For the tests, I didn't find yet a (not too ugly) way to connect as non-superuser like RDS and make the failing test which will pass thanks to this fix. I'll need to rework a bit the test provider for that.

But you can make similar two tests that I've done for the same behavior in database resource: https://github.com/terraform-providers/terraform-provider-postgresql/blob/master/postgresql/resource_postgresql_database_test.go#L186-L263

It tests that the temporary granted role is correctly revoked, or not revoked if connected user was actually already a member of the owner role.

cyrilgdn · 2019-04-08T21:20:32Z

postgresql/resource_postgresql_default_privileges.go

@@ -147,6 +163,18 @@ func resourcePostgreSQLDefaultPrivilegesDelete(d *schema.ResourceData, meta inte
 	}
 	defer deferredRollback(txn)

+	// Needed in order to set the owner of the db if the connection user is not a
+	// superuser
+	ownerGranted, err := grantRoleMembership(client.DB(), owner, currentUser)


Same comment as above.

cyrilgdn · 2019-04-08T21:20:38Z

postgresql/resource_postgresql_default_privileges.go

@@ -110,6 +112,18 @@ func resourcePostgreSQLDefaultPrivilegesCreate(d *schema.ResourceData, meta inte
 	}
 	defer deferredRollback(txn)

+	// Needed in order to set the owner of the db if the connection user is not a
+	// superuser
+	ownerGranted, err := grantRoleMembership(client.DB(), owner, currentUser)


In this case I think it would be great to use txn instead of creating a new connection (and so you can't use defer to call revokeRoleMembership but you can call it just before the txn.Commit();

Thanks to that this role will not be granted outside of the transaction.

You just need the type of the first parameter in both grantRoleMembership and revokeRoleMembership to accept sql.Tx or sql.Db.

I already wrote in another (not yet published) PR an interface for that (inspired from an issue in the database/sql lib):

// QueryAble is common interface for sql.DB and sql.Tx // (cf https://github.com/golang/go/issues/14468) type QueryAble interface { Exec(query string, args ...interface{}) (sql.Result, error) Query(query string, args ...interface{}) (*sql.Rows, error) QueryRow(query string, args ...interface{}) *sql.Row } [...] func grantRoleMembership(db QueryAble, [...])

I made it in the same way we already do that GRANT, I guess refactoring this to use txn could be made in another PR ?

It's done like that for the postgresql_database resource because databases cannot be created/dropped in a transaction, so there is no opened transactions where it's done.

But in this case all the queries we are doing are done in the transaction. So I would prefer using the transaction too for these queries.
But I can do it if you prefer.

cyrilgdn · 2019-04-08T21:32:46Z

postgresql/resource_postgresql_default_privileges.go

@@ -38,7 +38,7 @@ func resourcePostgreSQLDefaultPrivileges() *schema.Resource {
 				Type:        schema.TypeString,
 				Required:    true,
 				ForceNew:    true,
-				Description: "Role for which apply default privileges (You can change default privileges only for objects that will be created by yourself or by roles that you are a member of)",
+				Description: "The name of an existing role of which the current (connected user) role is a member (you can change default privileges only for objects that will be created by yourself or by roles that you are a member of)",


existing role of which the current (connected user) role is a member

Hmm actually no... (thanks to this PR)

My description was indeed not very clear but the connected user does not have to be a member of the owner role as we will, with this PR, temporary grant it just to apply default privilieges.

What the owner means:

If we have a database test which owner is test_owner and we have another role call readonly, we will write:

resource postgresql_default_privileges "read_only_tables" { role = "readonly" database = "test" schema = "public" owner = "test_owner" object_type = "table" privileges = ["SELECT"] }

So each time test_owner will create a table in this database (in schema public), readonly user will automatically have SELECT privileges on it.

But to apply this postgresql_default_privileges, the provider can be connected as a user (let's say postgres) which either is superuser so will have rights to apply it anyway, or is not superuser so the provider will temporary grant to postgres the test_owner role (if it's not already the case) to change these default privileges.

I copied that desc from Postgres docs.. and it is correct, but in our case it is correct only for a brief moment :) We should probably describe this temp grant role process in the docs.

It's correct for the point of view of a user executing the query. But as a user reading the provider documentation, it makes me think that the role I configured in the provider connection options needs to be a member of owner.
Which is not the case anymore thanks to this PR.

But maybe it's my bad English level which fails my comprehension 🤷‍♂️

zytek · 2019-05-27T12:11:31Z

But you can make similar two tests that I've done for the same behavior in database resource: https://github.com/terraform-providers/terraform-provider-postgresql/blob/master/postgresql/resource_postgresql_database_test.go#L186-L263

It tests that the temporary granted role is correctly revoked, or not revoked if connected user was actually already a member of the owner role.

Could we move that to come kind of commons/helper functions that would be tested once? No need to test the same behaviour multiple times?

cyrilgdn · 2019-05-31T11:54:37Z

Could we move that to come kind of commons/helper functions that would be tested once? No need to test the same behaviour multiple times?

Why not, I don't have an idea of how to do that yet, but if you have an idea for that feel free to propose 👍
Otherwise I think it's ok to copy paste the tests for now and we'll try to enhance this later (I have another resource that I need to create a PR for which uses the same pattern)

Signed-off-by: Jakub Paweł Głazik <zytek@nuxi.pl>

Make sure connected user has proper permissions to manage default privileges. Fixed hashicorp#70 Signed-off-by: Jakub Paweł Głazik <zytek@nuxi.pl>

So in this way this temporary grant is not even seen outside the transaction. This also adds a test to verify that owner is correctly revoked.

cyrilgdn

@zytek So i added a commit to your PR which:

Rebase on master.
Use the existing transactions to grant / revoke the owner to connected user. So this temporary privilege is not even seen outside of the transaction.
Add the little test that I talked about. It's not the best solution but at least it covered the code.
Change again the description of the owner field. Actually I talked with a co-worker about this description and we thought that even the field name is not very clear. So for now I set this little description but in the future we probably need to rename the field and complete/enhance the description (and especially in the website documentation).

I will quickly test that on RDS again to be sure it works then merge this PR. (If you want to take a look and approve / comment / request changes before, feel free!)

Thanks again for your work 👍 !

zytek · 2019-07-25T12:52:18Z

@cyrilgdn hey Cyril, thanks for looking into it and moving it forward.

I just did a recompile with your changes and ran a quick test on my TF setup. Works fine.

Apply complete! Resources: 14 added, 0 changed, 0 destroyed.

ghost added the size/S label Apr 2, 2019

zytek changed the title ~~Grant owner role to connected user~~ Grant owner role to connected user for default privileges management Apr 2, 2019

cyrilgdn suggested changes Apr 8, 2019

View reviewed changes

zytek and others added 3 commits July 3, 2019 21:58

Update resource description in line with PostgreSQL docs

71db727

Signed-off-by: Jakub Paweł Głazik <zytek@nuxi.pl>

Grant owner role to connected user

7ea3b20

Make sure connected user has proper permissions to manage default privileges. Fixed hashicorp#70 Signed-off-by: Jakub Paweł Głazik <zytek@nuxi.pl>

default privileges: grant & revoke the owner in the transaction.

b0f345b

So in this way this temporary grant is not even seen outside the transaction. This also adds a test to verify that owner is correctly revoked.

cyrilgdn force-pushed the fix-70 branch from 3e6c1b5 to b0f345b Compare July 11, 2019 17:39

ghost added size/L and removed size/S labels Jul 11, 2019

cyrilgdn approved these changes Jul 12, 2019

View reviewed changes

cyrilgdn merged commit a64fd42 into hashicorp:master Aug 2, 2019

mythus mentioned this pull request Feb 23, 2020

Temporary permissions conflict with IAM authentication #120

Open

rimmington mentioned this pull request May 20, 2020

Grant owner role to connected user for schema alteration #139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grant owner role to connected user for default privileges management #71

Grant owner role to connected user for default privileges management #71

zytek commented Apr 2, 2019

zytek commented Apr 2, 2019

cyrilgdn left a comment •

edited

Loading

cyrilgdn Apr 8, 2019

cyrilgdn Apr 8, 2019

zytek May 27, 2019

cyrilgdn May 31, 2019

cyrilgdn Apr 8, 2019 •

edited

Loading

zytek May 27, 2019

cyrilgdn May 31, 2019

zytek commented May 27, 2019

cyrilgdn commented May 31, 2019

cyrilgdn left a comment •

edited

Loading

zytek commented Jul 25, 2019

Grant owner role to connected user for default privileges management #71

Grant owner role to connected user for default privileges management #71

Conversation

zytek commented Apr 2, 2019

zytek commented Apr 2, 2019

cyrilgdn left a comment • edited Loading

Choose a reason for hiding this comment

cyrilgdn Apr 8, 2019

Choose a reason for hiding this comment

cyrilgdn Apr 8, 2019

Choose a reason for hiding this comment

zytek May 27, 2019

Choose a reason for hiding this comment

cyrilgdn May 31, 2019

Choose a reason for hiding this comment

cyrilgdn Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

zytek May 27, 2019

Choose a reason for hiding this comment

cyrilgdn May 31, 2019

Choose a reason for hiding this comment

zytek commented May 27, 2019

cyrilgdn commented May 31, 2019

cyrilgdn left a comment • edited Loading

Choose a reason for hiding this comment

zytek commented Jul 25, 2019

cyrilgdn left a comment •

edited

Loading

cyrilgdn Apr 8, 2019 •

edited

Loading

cyrilgdn left a comment •

edited

Loading