Skip to content

Conversation

Vampouille
Copy link
Contributor

To generate this kind of query:

REVOKE CONNECT ON DATABASE database1 FROM PUBLIC;

I created following puppet code:

postgresql::server::database_grant { 'revoke connect on database1':
    ensure => 'absent',
    db => 'database1',
    privilege => 'CONNECT',
    role => 'PUBLIC',
}

This generate following error:

Error: /Stage[main]/Profile::Postgresql_gis/Postgresql::Server::Grant[revoke ALL on schema public to public:]/Postgresql_psql[grant:revoke ALL on schema public to public:]: Could not evaluate: Error evaluating 'unless' clause, returned pid 7360 exit 1: 'ERROR:  role "PUBLIC" does not exist

because PUBLIC is a "implicit" role and is not listed in pg_roles table.

To fix this I modify query that test if role exists.

@Vampouille
Copy link
Contributor Author

Puppet call the SQL function : has_database_privilege() to check privileges. This function works with 'PUBLIC' role but you need to specify 'PUBLIC' as lowercase : 'public'. See https://www.postgresql.org/docs/9.1/static/functions-info.html

When using System Information Functions like:
* has_database_privilege()
* has_table_privilege()

Allow usage of 'PUBLIC' in GRANT/REVOKE definition :

REVOKE CONNECT ON DATABASE database1 FROM PUBLIC;

postgresql::server::database_grant { 'revoke connect on database1':
    ensure => 'absent',
    db => 'database1',
    privilege => 'CONNECT',
    role => 'PUBLIC',
}
@Vampouille Vampouille force-pushed the allow_usage_of_public_role branch from f9012da to 6787f00 Compare July 17, 2018 07:58
@Vampouille
Copy link
Contributor Author

@david22swan Can you take a look at this PR ? Is there anything I can do to help merging this PR ?

@david22swan
Copy link
Member

@Vampouille Sorry for your PR being left to lie for so long.
In order to get this merged we would require test coverage to be added to ensure that your change works correctly and allow us to catch any error's that occur to it in the future, and for the documentation to be updated to include a reference to it.

@david22swan
Copy link
Member

@Vampouille Sorry to bother you but is there any movement on this PR???

@david22swan
Copy link
Member

@Vampouille Apologies but due to the amount of time this has been left to lie fallow and your lack of response when prompted I am now closing this PR. If at a later time you decide to return to this PR, or create a new one, we will of course be more than happy to once again review your work.
Thank you for taking the time to create this PR even if it has not been merged.
Best Wishes

@Vampouille
Copy link
Contributor Author

@david22swan Can you reconsider this PR ? I don't known how to write units tests and even don't known what to test is this case. Do you have some link to other PR that add tests for the database_grant resource ?
Additionally, this PR does not add a new feature, it just fix a bug in this puppet module:

With postgres this works :

REVOKE CONNECT ON DATABASE application_db FROM PUBLIC;

With this puppet module this does not work:

postgresql::server::database_grant { 'revoke connect on application_db':                                                                                                   
  ensure    => 'absent',
  db        => 'application_db',
  privilege => 'CONNECT',
  role      => 'PUBLIC',
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants