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 user password lock option to user module #37962

Merged
merged 7 commits into from
Apr 19, 2018

Conversation

lazouz
Copy link
Contributor

@lazouz lazouz commented Mar 26, 2018

SUMMARY

This adds password lock feature (lock -l command) tu user module.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module : user

ANSIBLE VERSION
ansible 2.4.3.0
ADDITIONAL INFORMATION

I have not written tests for this feature, because i don't know if it can be useful

fixes #29466

@lazouz lazouz force-pushed the add_user_lock_password branch 2 times, most recently from e52e0fd to c027b90 Compare March 26, 2018 21:48
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2018

cc @sfromm
click here for bot help

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 26, 2018
@lazouz lazouz force-pushed the add_user_lock_password branch 2 times, most recently from 49b891b to 6da3124 Compare March 26, 2018 21:56
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:178:161: E501 line too long (169 > 160 characters)

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/system/user.py:0:0: E309 version_added for new option (password_lock) should be 2.6. Currently 2.4
lib/ansible/modules/system/user.py:0:0: E325 argument_spec for "password_lock" defines type="bool" but documentation does not

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 26, 2018
@@ -173,6 +173,10 @@
- An expiry time for the user in epoch, it will be ignored on platforms that do not support this.
Currently supported on Linux, FreeBSD, and DragonFlyBSD.
version_added: "1.9"
password_lock:
description:
- lock the password (usermod -L ) by adding a '!' at the beggining of the password user entry. This option does not disable the user, only lock the password.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/beggining/beginning/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

password_lock:
description:
- lock the password (usermod -L ) by adding a '!' at the beggining of the password user entry. This option does not disable the user, only lock the password.
version_added: "2.4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version_added: "2.6"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@Akasurde Akasurde removed the needs_triage Needs a first human triage before being processed. label Mar 27, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Mar 27, 2018

I did what @ansibot told me :)

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 27, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 27, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:178:107: W291 trailing whitespace

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Mar 27, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Mar 27, 2018

ooops. I fixed.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 27, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Mar 27, 2018

now i only need @Akasurde validation ;)

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Apr 4, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Apr 4, 2018

hello @Akasurde
I think I've done what you asked for !
If you have some time, can you give a glance to the code ?

cheers

@ansibot
Copy link
Contributor

ansibot commented Apr 16, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:176:47: W291 trailing whitespace

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 16, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 17, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Apr 17, 2018

hey @Akasurde @bcoca is everything all right for you now ?

@@ -909,6 +922,9 @@ def modify_user(self):
cmd.append('-e')
cmd.append(str(int(days)))

if self.password_lock:
cmd.append('-L')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is incorrect for freebsd, -L refers to login class, not password lock, just look at a few lines above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewrited the lock/unlock commands for FreeBSD and NetBSD, finally, and updated the doc.

@ansibot
Copy link
Contributor

ansibot commented Apr 17, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:0: syntax-error unindent does not match any outer indentation level (<string>, line 975)

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: SyntaxError: if self.uid is not None and info[2] != int(self.uid):

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: SyntaxError: if self.uid is not None and info[2] != int(self.uid):

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: SyntaxError: if self.uid is not None and info[2] != int(self.uid):

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: SyntaxError: if self.uid is not None and info[2] != int(self.uid):

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: SyntaxError: if self.uid is not None and info[2] != int(self.uid):

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 55, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test import --python 2.7 [explain] failed with 2 errors:

lib/ansible/modules/system/user.py:0:0: IndentationError: unindent does not match any outer indentation level (user.py, line 975) (in /root/ansible/test/runner/.tox/minimal-py27/bin/importer.py:82)
lib/ansible/modules/system/user.py:975:66: IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test import --python 3.5 [explain] failed with 2 errors:

lib/ansible/modules/system/user.py:0:0: IndentationError: unindent does not match any outer indentation level (user.py, line 975) (in /root/ansible/test/runner/.tox/minimal-py35/bin/importer.py:82)
lib/ansible/modules/system/user.py:975:66: IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test import --python 3.6 [explain] failed with 2 errors:

lib/ansible/modules/system/user.py:0:0: IndentationError: unindent does not match any outer indentation level (user.py, line 975) (in /root/ansible/test/runner/.tox/minimal-py36/bin/importer.py:82)
lib/ansible/modules/system/user.py:975:66: IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test import --python 3.7 [explain] failed with 2 errors:

lib/ansible/modules/system/user.py:0:0: IndentationError: unindent does not match any outer indentation level (user.py, line 975) (in /root/ansible/test/runner/.tox/minimal-py37/bin/importer.py:82)
lib/ansible/modules/system/user.py:975:66: IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/system/user.py:969:14: E111 indentation is not a multiple of four
lib/ansible/modules/system/user.py:970:17: E121 continuation line under-indented for hanging indent
lib/ansible/modules/system/user.py:974:13: E122 continuation line missing indentation or outdented
lib/ansible/modules/system/user.py:975:13: E901 IndentationError: unindent does not match any outer indentation level

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/system/user.py:0:0: E401 Python SyntaxError while parsing module
test/sanity/validate-modules/ignore.txt:1685:1: A102 Remove since "lib/ansible/modules/system/user.py" passes "E324" test
test/sanity/validate-modules/ignore.txt:1686:1: A102 Remove since "lib/ansible/modules/system/user.py" passes "E327" test

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/system/user.py:975:66: python-syntax-error unindent does not match any outer indentation level (<unknown>, line 975)

click here for bot help

Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 19, 2018
@lazouz
Copy link
Contributor Author

lazouz commented Apr 19, 2018

Thank you very much @bcoca @Akasurde for your time, your reviews and your validation !
It was my first contribution to an open source project, I think it won't be the last :) .

@bcoca
Copy link
Member

bcoca commented Apr 19, 2018

@lazouz just need to fix the CI errors and we can merge this, ... refreshed page... thank you for contributing!

@bcoca bcoca merged commit a1759b0 into ansible:devel Apr 19, 2018
@Akasurde
Copy link
Member

@lazouz Thanks for your contribution.

oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018
* add user password lock option to user module

* fixup! add user password lock option to user module

* add unlock, set no default

* fixup! add unlock, set no default

* fixup! fixup! add unlock, set no default

* add lock password for FreeBSD, netBSD

* fixup! add lock password for FreeBSD, netBSD
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018
* add user password lock option to user module

* fixup! add user password lock option to user module

* add unlock, set no default

* fixup! add unlock, set no default

* fixup! fixup! add unlock, set no default

* add lock password for FreeBSD, netBSD

* fixup! add lock password for FreeBSD, netBSD
@gaddman
Copy link
Contributor

gaddman commented Jul 15, 2018

Nice update to the module, I've just started using it. Noticed that it's not idempotent though. Do you have plans to add that? Just did some testing and shouldn't take much, something like this (at line 659) :

        if self.password_lock:
            if info[1] and info[1][0] != '!':
                cmd.append('-L')
        elif not self.password_lock:
            if info[1] and info[1][0] == '!':
                cmd.append('-U')

@lazouz
Copy link
Contributor Author

lazouz commented Jul 18, 2018

The problem is : the behavior is different on every Operating System, the solution you have works for linux only. We have to find a solution for FreeBSD, etc.

@gaddman
Copy link
Contributor

gaddman commented Jul 18, 2018

Yeah, there's a bit more to it for sure. Just checking FreeBSD docs, they prepend *LOCKED*, so it's still possible there at least. Also, the code I wrote above is missing logic for the case where no password is present. I'll have a bash at writing a PR in the next couple of weeks.

ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
* add user password lock option to user module

* fixup! add user password lock option to user module

* add unlock, set no default

* fixup! add unlock, set no default

* fixup! fixup! add unlock, set no default

* add lock password for FreeBSD, netBSD

* fixup! add lock password for FreeBSD, netBSD
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user module: add parameter for --disabled-password option
5 participants