Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commas separated privileges for default_privileges doesn't work as expected. #1438

Open
momo57420 opened this issue May 17, 2023 · 2 comments

Comments

@momo57420
Copy link

Describe the Bug

I tried to set several privileges as default privileges for a role in a schema but it failed in error during execution (Illegal value for $privilege parameter.)

Expected Behavior

ALTER DEFAULT PRIVILEGES FOR ROLE xyz_dml IN SCHEMA xyz_sch GRANT INSERT,SELECT,UPDATE,DELETE ON TABLES TO "xyz_aoo"

Steps to Reproduce

Hi there,

I have created a type called t_default_privileges

type Db_profile::Postgresql::T_default_privileges =
  Struct[
    user => String,
    ensure => Enum['present','absent'],
    db => String,
    owner => Optional[String],
    privilege => String,
    schema => String,
    object_type => Enum['FUNCTIONS','ROUTINES','SEQUENCES','TABLES','TYPES'],
  ]

Then i have created a class default_privileges

class db_profile::postgresql::server::default_privileges (
  Boolean $is_primary = $db_profile::postgresql::server::is_primary,
  Optional[Array[Db_profile::Postgresql::T_default_privileges]]
    $default_privileges      = $db_profile::postgresql::server::default_privileges,
) {
  Anchor['postgresql::server::service::end']
  -> Class['db_profile::postgresql::server::default_privileges']

  if $is_primary {
    each($default_privileges) |$default_privilege| {

      notify {"Running DDP ----> Custom ------> $default_privilege":}

      if $default_privilege['ensure'] == 'present' {
        postgresql::server::default_privileges{$default_privilege['user']:
          target_role => $default_privilege['owner'],
          ensure => 'present',
          db  => $default_privilege['db'],
          role   => $default_privilege['user'],
          privilege => $default_privilege['privilege'],
          schema => $default_privilege['schema'],
          object_type => $default_privilege['object_type'],
        }
       }else{
        postgresql::server::default_privileges{$default_privilege['user']:
          target_role => $default_privilege['owner'],
          ensure => 'absent',
          db  => $default_privilege['db'],
          role   => $default_privilege['user'],
          privilege => $default_privilege['privilege'],
          schema => $default_privilege['schema'],
          object_type => $default_privilege['object_type'],
        }
      }
    }
  }
}

Then in hiera, i have defined

db_profile::postgresql::server::default_privileges:
    - user: xyz_aoo
      ensure: present
      db: xyz
      privilege: INSERT,SELECT,UPDATE,DELETE
      owner: xyz_dml 
      schema: xyz_sch
      object_type: TABLES

The result is that it goes directly in default: { fail('Illegal value for $privilege parameter') } while testing $_privilege in /manifest/servers/default_privileges.pp

    'TABLES': {
      case $_privilege {
        /^ALL$/: { $_check_privilege = 'arwdDxt' }
        /^DELETE$/: { $_check_privilege = 'd' }
        /^INSERT$/: { $_check_privilege = 'a' }
        /^REFERENCES$/: { $_check_privilege = 'x' }
        /^SELECT$/: { $_check_privilege = 'r' }
        /^TRIGGER$/: { $_check_privilege = 'd' }
        /^TRUNCATE$/: { $_check_privilege = 'D' }
        /^UPDATE$/: { $_check_privilege = 'w' }
        default: { fail('Illegal value for $privilege parameter') }
      }

It seems that the regexp used does not match expression with comma separated values if $_privilege is build like priv1,priv2,priv3 etc.

If i change fail by notify in default then there is a problem with the unless command as it has not retrieved the correct $_check_privilege variable.
But the grant_command is correct with ALTER DEFAULT PRIVILEGES FOR ROLE xyz_dml IN SCHEMA xyz_sch GRANT INSERT,SELECT,UPDATE,DELETE ON TABLES TO "xyz_aoo"'

However, it is stated in the header
# @param privilege Specifies comma-separated list of privileges to grant. Valid options: depends on object type.

With only one privilege it is working, with several ones separated by commas, it doesnt 't work.

Am i doing something wrong ?

Thanks

Environment

Red Hat 8.7
Postgres 13

@jordanbreen28
Copy link
Contributor

Hi @momo57420 - Thanks for raising this, this is interesting and looks like a genuine bug with the "$privilege" parameter.

Just from a quick glance at a high level, i would say the '$privilege' parameter and respective documenation needs updating to also accept an array of strings and perform some logic to join those privileges as a single comma-separated list. I'll label this issue so it can get properly prioritised by the team (or if you're feeling up to it, we are accepting PRs).

Thanks again.

@jordanbreen28 jordanbreen28 self-assigned this May 17, 2023
@momo57420
Copy link
Author

momo57420 commented May 22, 2023

Hi there,

In the meantime, as a workaround, I replaced in manifest/servers/default_privileges.pp the regexp with this one :

    'TABLES': {
      case $_privilege {
        /^ALL$/: { $_check_privilege = 'arwdDxt' }
        /(SELECT|INSERT|UPDATE|DELETE|REFERENCES|TRIGGER|TRUNCATE)(,)*/: { $_check_privilege = 'rwdDxt' }
        #/^DELETE$/: { $_check_privilege = 'd' }
        #/^INSERT$/: { $_check_privilege = 'a' }
        #/^REFERENCES$/: { $_check_privilege = 'x' }
        #/^SELECT$/: { $_check_privilege = 'r' }
        #/^TRIGGER$/: { $_check_privilege = 'd' }
        #/^TRUNCATE$/: { $_check_privilege = 'D' }
        #/^UPDATE$/: { $_check_privilege = 'w' }
        default: { fail('Illegal value for $privilege parameter') }
      }
      $_check_type = 'r'

The only problem is with $check_privilege, it should be set with a combination of privileges declared.
Maybe with a split function to construct an array and test each position of the array.

But even if the $check_privilege is not correct, it is an unless cmd.
So it will execute the sql command anyway as the unless cmd is false.

Thanks

@jordanbreen28 jordanbreen28 removed their assignment Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants