Improve the ldap_ban playbook

hashbang / admin-tools

Ansible playbooks and other admin tools used to administrate #! servers

MIT License

17 stars 10 forks source link

Improve the ldap_ban playbook #141

Closed KellerFuchs closed 5 years ago

KellerFuchs commented 5 years ago

[x] Use ldap_attr rather than running a raw command on ldap.hashbang.sh.
[x] Ignore errors from sss_cache. It turns out it can spuriously error-out. Wheee, quality software \o/
[x] Support banning multiple users at once.
[x] Support (optionally) deleting the users' home directories.
[x] Prompt the administrator interactively for options.

KellerFuchs commented 5 years ago

@dpflug No, it needs python-ldap on your machine. (apt install python-ldap on Debian)

KellerFuchs commented 5 years ago

@hashbang/administrators If that's ready to merge, I will rebase and sign, and people who reviewed can sign/merge/...

@RyanSquared, @dpflug Thanks for testing the actual code. @dpflug Can you still reproduce the failure you had? I have no idea why it would be failing, complaining about Python strings...

@benharri Thanks for checking the README <3

dpflug commented 5 years ago

Smells like my issues are local. https://gist.github.com/dpflug/2e51ea23b71a282d4cc961966cec17a9

dpflug commented 5 years ago

https://github.com/ansible/ansible/issues/39569 - Found the issue. It's already fixed by PR. I just need to wait for Debian to update Ansible. :skull:

dpflug commented 5 years ago

Wait, this laptop is Arch. I'm waiting on Ansible 2.7 to release.

That should be much quicker.

KellerFuchs commented 5 years ago

@dpflug OK; for a moment I was confused why this doesn't happen here (Ansible 2.6.3 on Debian testing), but it's a Py2 vs. Py3 issue (Py3 support in Ansible still isn't super-good)

KellerFuchs commented 5 years ago

@hashbang/administrators That should be ready to go. If some of the people who reviewed could merge & sign, that would be good.

RyanSquared commented 5 years ago

@KellerFuchs PR is still open (sorry!) if you want to amend the docs.

KellerFuchs commented 5 years ago

@RyanSquared Indeed, only README.md had been updated. FWIW, doc/Blocking account mostly has documentation explaining what the playbook does, but it was still worth updating the “how to run” part.

KellerFuchs commented 5 years ago

@RyanSquared Done.

RyanSquared commented 5 years ago

GitHub isn't showing it as merged, but it is.

KellerFuchs commented 5 years ago

@RyanSquared What the heck happened? Did you rebase before merging?

KellerFuchs commented 5 years ago

FYI, the way to do that is rebase the PR branch, (force) push it, then merge to master and push the merge commit.

KellerFuchs commented 5 years ago

Merge seems OK:

$ git diff --stat ldap_ban 
 roles/coreos-authorized_keys  |  0
 terraform/GNUmakefile         | 21 ---------------------
 terraform/README.md           | 51 +++++++++++++++++++++++----------------------------
 terraform/modules/r53/main.tf |  5 +++--
 4 files changed, 26 insertions(+), 51 deletions(-)

RyanSquared commented 5 years ago

@RyanSquared What the heck happened? Did you rebase before merging?

I didn't think I did, but I may have.