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

Add locked state for user module, beginnings of integration test #6303

Closed
wants to merge 7 commits into from

Conversation

billwanjohi
Copy link
Contributor

This branch adds implementations for locking user accounts across systems, but returns an error for all but Generic (Linux) systems, since I'm not able to test the others. Guided by #2211.

It also has basic tests for creating and deleting a user.

Lastly, there's a skipped test that would check for a locked password, but I'm unconvinced that that is necessary once you've expired the account.

I took these from man pages I found online,
but since I don't have these systems to test,
I'll leave the failure if platform != Generic
@mpdehaan
Copy link
Contributor

mpdehaan commented Mar 6, 2014

Thanks for this.

(Since this makes users on the system where the tests are run, this probably needs to be moved to the "destructive" integration test category. Can you update this?)

I'll leave testing for others when we get to this in the queue.

Thanks again!

@billwanjohi
Copy link
Contributor Author

That makes sense, moved it to destructive.

@quiver
Copy link
Contributor

quiver commented Mar 15, 2014

It looks to me what @billwanjohi implementted here is setting expire date for user accounts($ useradd --expiredate), rather than making it locked.
I think #2211 is to implement $ usermod --lock/--unlocked in ansible, doesn't it?

@billwanjohi
Copy link
Contributor Author

You are correct about what's implemented here. The problem with --lock is that it only affects the password. Specifically, it prepends an "!" in front of the user's stored password, thus breaking it. Any other type of access is permitted.

I think that that is inadequate and deceptive, and so the account should be expired.

@quiver
Copy link
Contributor

quiver commented Mar 17, 2014

The problem with --lock is that it only affects the password.

That's right. Locked users still can log in with key authentications.

I think that that is inadequate and deceptive, and so the account should be expired

lock and expire are two different thing. lock-ed account may behave counterintuitive but this is lock's specification. If you want to lock an account, you can make the account locked state. If you want to expire an account, you can make the account expired state. Locking an account ends up with expiring it seems odd to me.

FYI, chef's user lock is usermod --lock internally. http://docs.opscode.com/resource_user.html

Anyway, I have no objection to adding expire state to user module.

@billwanjohi
Copy link
Contributor Author

I am not opposed to the state being named expired.

@mpdehaan mpdehaan added the P3 label Mar 19, 2014
@mpdehaan
Copy link
Contributor

Sounds like the argument is for adding "expired" in addition to lock, please make this so and I'll see about merging this.

Meanwhile, we should also make very sure the description for each field makes this clear, so users don't think they have locked something completely when key access is still available.

Expiring a Unix account prevents any kind of access, and so seems
more appropriate as a *user* state change compared to locking,
which only prevents usage of the password.

But, since some admins will expect this less stringent behavior, I've
renamed this state to be clear what the usermod command will do.
@billwanjohi
Copy link
Contributor Author

OK, I renamed the state to expired, and added an explanatory sentence to the user state documentation. Tests still pass for me.

I did not implement the locked or unlocked state, because it seems either redundant or insufficient in almost all cases. But I did leave a (when: no) task in the user integration test, if someone wanted to add that later. If that were implemented, I would suggest it be a separate user option altogether, maybe "password_lock" with yes and no choices.

All that said, does this look good to merge?

@mpdehaan mpdehaan added P4 and removed P3 labels Apr 17, 2014
@mpdehaan
Copy link
Contributor

Hi!

Thanks very much for your interest in Ansible. It sincerely means a lot to us.

On September 26, 2014, due to enormous levels of contribution to the project Ansible decided to reorganize module repos, making it easier
for developers to work on the project and for us to more easily manage new contributions and tickets.

We split modules from the main project off into two repos, http://github.com/ansible/ansible-modules-core and http://github.com/ansible/ansible-modules-extras

If you still would like this pull request merged, we will need your help making this target the new repo. If you do not take any action, this
pull request unfortunately cannot be applied.

We apologize that we are not able to make this transition happen seamlessly, though this is a one-time change and your help is greatly appreciated --
this will greatly improve velocity going forward.

Both sets of modules will ship with Ansible, though they'll receive slightly different ticket handling.

To locate where a module lives between 'core' and 'extras'

Otherwise, if this is a new module:

It may be possible to re-patriate your pull requests automatically, one user-submitted approach for advanced git users
has been suggested at https://gist.github.com/willthames/afbaaab0c9681ed45619

Additionally, should you need more help with this, you can ask questions on:

Thank you very much!

@mpdehaan mpdehaan closed this Sep 29, 2014
billwanjohi added a commit to billwanjohi/ansible-modules-core that referenced this pull request Sep 29, 2014
ported from
ansible/ansible#6303

It's very useful and routine to disable a *nix user.
I implemented expired instead of locked because this prevents any use of
the account, safer than just preventing password-based authentication.

I have tests [1], but since none of the suite came along with the core
modules, I'm unsure how to submit them.

[1] https://github.com/billwanjohi/ansible/blob/add_locked_state/test/integration/roles/test_user/tasks/main.yml
@billwanjohi billwanjohi deleted the add_locked_state branch July 28, 2015 17:53
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants