search

ghoneycutt / puppet-module-pam

Puppet module to manage PAM
Other
18 stars 78 forks source link

Explicitly Denied Users in access.conf #119

Closed Jetroid closed 5 years ago

Jetroid commented 9 years ago

Hi ghoneycutt,

My organisation needed to be able to have a list of users who are explicitly denied in access.conf even if they are in an allowed group. I couldn't see support for this anywhere in your README so I've provided support for a denied_users (baddies list) parameter. Users or groups in this list will be denied regardless of if they are in the allowed groups.

I wanted to make the changes to access.conf.erb DRY, but I'm relatively new to using erb templates and using a 'def' caused me no end of errors where it seemed the interpreter was parsing the template wrong, so I decided to drop it for this which is functional. Perhaps you know a better way?

I've modified your provided tests to incorporate the denied_users list. I understand that in some of the later tests you used .with_content(), (which I presume is because of the lack of consistent hash ordering), and this means that I couldn't test that the denied_users came before the allowed_users in the file without making the tests unreadable, which is not ideal. Otherwise the tests should all be fine and cover all cases.

Thanks for the good work with the modules! Regards, Jetroid.

ghoneycutt commented 8 years ago

@Phil-Friderici what do you think?

Phil-Friderici commented 8 years ago

@Jetroid Thanks for your work !

@ghoneycutt Looks good to me. :+1: The added functionality makes sense. Code & tests are corresponding.

I've modified your provided tests to incorporate the denied_users list. I understand that in some of the later tests you used .with_content(), (which I presume is because of the lack of consistent hash ordering), and this means that I couldn't test that the denied_users came before the allowed_users in the file without making the tests unreadable, which is not ideal. Otherwise the tests should all be fine and cover all cases.

From my perspective it's good enough to have one full example which proofs the right sequence [1]. So I appreciate using the simple and readable .with_content() tests afterwards.

Maybe the wording in the template should get better alligned: deny any users listed here allow only the groups listed

[1] https://github.com/Jetroid/puppet-module-pam/blob/master/spec/classes/accesslogin_spec.rb#L110-L123

Jetroid commented 8 years ago

Hi @ghoneycutt, @Phil-Friderici, I have made the wording more consistent. See latest commit.

ghoneycutt commented 5 years ago

We have done a major version upgrade to support Puppet 5 and 6. This involves the transition to hiera data in modules, acceptance testing, type validation in parameters and more.

Your contribution is greatly appreciated! Please check it out the new version 3 and re-open if needed.