search

hashicorp / terraform-provider-postgresql

As part of our introduction to self-service publishing in the Terraform Registry, this copy of the provider has been archived, and ownership has been transferred to active maintainers in the community. Please see the new location on the Terraform Registry: https://registry.terraform.io/providers/cyrilgdn/postgresql
https://github.com/cyrilgdn/terraform-provider-postgresql
Mozilla Public License 2.0
103 stars 79 forks source link

Fix withRolesGranted permissions troubles. #162

Closed cyrilgdn closed 4 years ago

cyrilgdn commented 4 years ago

This fix #157 and #161

cyrilgdn commented 4 years ago

@multani Thanks for the review !

I think I would also try to extract 2 functions, that you are repeating, in slightly different ways:

* One function to revoke a set of roles from an user and return the list of roles actually revoked

* One function to do the opposite: assign a set of roles to an user, returning what has been actually assigned.

It DRY a bit the code and maybe make the intention clearer in the big withRolesGranted function.

I see your point even if in the revoke case, it would need 2 different functions, one to revoke a list roles from a user, and one to revoke a list of members from a role.

It could still be helpful to create these functions but, as we still need to range over roles to check if they are superuser, for now I'd let it like that if you don't mind. I still have simplied a bit by making revokeRoleMembership more similar than grantRoleMembership (i.e.: the function calls isMemberOfRole and returns a boolean which tells if the revoke was necessary, cf https://github.com/terraform-providers/terraform-provider-postgresql/pull/162/commits/d68c32c2a20717addf91bd9a786ef18a962cc16a#diff-100cd0e9e0a4fe44e9c363dc9e18600fL149-L156)