Only change expiration date if it is different #38885

samdoran · 2018-04-17T14:53:51Z

SUMMARY

Modify user_info() method to also return the password expiration.
Compare current and desired expiration times and only change if they are different.

Fixes #13235

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

user.py

ANSIBLE VERSION

2.5

ansibot · 2018-04-17T14:56:51Z

cc @sfromm
click here for bot help

ansibot · 2018-04-17T16:08:55Z

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

test/integration/targets/user/tasks/main.yml:198:1: empty-lines too many blank lines (1 > 0)

click here for bot help

webknjaz · 2018-04-17T16:29:55Z

-label needs_triage

ansibot · 2018-04-17T17:02:38Z

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

test/integration/targets/user/tasks/main.yml:198:1: empty-lines too many blank lines (1 > 0)

click here for bot help

bcoca · 2018-04-20T23:08:31Z

@samdoran, i believe the failing tests are wrong

samdoran · 2018-04-24T15:09:45Z

@bcoca Looks like some tests are incorrect, others are legit.

I need to skip for macOS. The failure on CentOS 6 with Python 2.6 is legit: no totalseconds() method ☹️. Guess I'll have to do that differently.

Also, I need to add expiration change logic for FreeBSD as well. I missed that since I only fixed the method in the User class.

Some of the tests, I'm not sure what is triggering the change on the second task run. RHEL7.4/1 for example.

bcoca · 2018-04-24T20:11:40Z

lib/ansible/modules/system/user.py

@@ -271,6 +270,7 @@ class User(object):
    platform = 'Generic'
    distribution = None
    SHADOWFILE = '/etc/shadow'
+    SHADOWFILE_INDEX = 7


shadowfile_expire_index? .. just index seems ambiguous and we might need others in future

Very good point.

bcoca · 2018-04-25T17:09:03Z

test/integration/targets/user/tasks/main.yml

-
-##
+    that:
+      - "not user_test1.results[0]['changed']"


the proper test would be is changed

samdoran · 2018-04-25T19:14:26Z

@bcoca So the failing tests revealed a couple problems:

I used the incorrect date math functions that mix localtime and UTC.
The timezone integration tests change the timezone on the system and don't set it back to UTC.

So the untidy timezone test actually exposed a bug in my fix. Lucky for me.

I'm working on getting this fixed now that I've been able to figure out what's going on (with a ton of help from @mattclay).

Modify user_info() method to also return the password expiration. Compare current and desired expiration times and only change if they are different.

Skip macOS and use getent module for validating expiration date.

…CentOS 6

Use separate tasks for verifying expiration date on BSD

calendar.timegm() is the inverse of time.gmtime() and returns a timestamp in UTC not localtime Add tests that change the system timezone away from UTC

mattclay · 2018-04-26T06:43:55Z

CI failure in FreeBSD integration tests:

https://app.shippable.com/github/ansible/ansible/runs/62723/26/tests
https://app.shippable.com/github/ansible/ansible/runs/62723/27/tests

The other failures are unrelated to this PR.

Use DATE_FORMAT when setting expiration date on FreeBSD. Previously the argument passed to -e was an integer of days since epoch when the account will expire which was inserted directly into master.passwd. This value is interpreted as seconds since epoch by the system, meaning the account expiration was actually set to a few hours past epoch. Greatly simply comparing desired and current expiration time by using the first three values of the struct_time tuple rather than doing a whole bunch of manipulations of the seconds since epoch.

samdoran · 2018-04-30T18:10:48Z

@bcoca I think I finally got this working.

I found a bug in the FreeBSD class. Passing an int to -e sets that directly in /etc/master.passwd:

root@ip-192-168-3-46:~/ansible$ grep ansi /etc/master.passwd
ansibulluser:*:1002:1002::0:6221:User &:/home/ansibulluser:/bin/sh
                            ^
                            This is seconds since epoch

The man pages aren't clear and some of them do say an integer in DAYS or a date string. I switched to using the date string since that seems to behave correctly.

* Only change expiration date if it is different Modify user_info() method to also return the password expiration. Compare current and desired expiration times and only change if they are different. * Improve formatting on user tests * Add integration test for expiration * Add changelog fragment * Improve integration test Skip macOS and use getent module for validating expiration date. * Fix expiration change for FreeBSD * Don't use datetime since the total_seconds method isn't available on CentOS 6 * Use better name for expiration index field Use separate tasks for verifying expiration date on BSD * Use calendar.timegm() rather than time.mktime() calendar.timegm() is the inverse of time.gmtime() and returns a timestamp in UTC not localtime Add tests that change the system timezone away from UTC * Mark tests as destructive and use test for change status * Fix account expiration for FreeBSD Use DATE_FORMAT when setting expiration date on FreeBSD. Previously the argument passed to -e was an integer of days since epoch when the account will expire which was inserted directly into master.passwd. This value is interpreted as seconds since epoch by the system, meaning the account expiration was actually set to a few hours past epoch. Greatly simply comparing desired and current expiration time by using the first three values of the struct_time tuple rather than doing a whole bunch of manipulations of the seconds since epoch. (cherry picked from commit 5a6bdef)

* Only change expiration date if it is different Modify user_info() method to also return the password expiration. Compare current and desired expiration times and only change if they are different. * Improve formatting on user tests * Add integration test for expiration * Add changelog fragment * Improve integration test Skip macOS and use getent module for validating expiration date. * Fix expiration change for FreeBSD * Don't use datetime since the total_seconds method isn't available on CentOS 6 * Use better name for expiration index field Use separate tasks for verifying expiration date on BSD * Use calendar.timegm() rather than time.mktime() calendar.timegm() is the inverse of time.gmtime() and returns a timestamp in UTC not localtime Add tests that change the system timezone away from UTC * Mark tests as destructive and use test for change status * Fix account expiration for FreeBSD Use DATE_FORMAT when setting expiration date on FreeBSD. Previously the argument passed to -e was an integer of days since epoch when the account will expire which was inserted directly into master.passwd. This value is interpreted as seconds since epoch by the system, meaning the account expiration was actually set to a few hours past epoch. Greatly simply comparing desired and current expiration time by using the first three values of the struct_time tuple rather than doing a whole bunch of manipulations of the seconds since epoch.

samdoran requested a review from bcoca April 17, 2018 14:53

samdoran force-pushed the issue/13235-user-expires branch from f3204f3 to bc26623 Compare April 17, 2018 15:56

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. test This PR relates to tests. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 17, 2018

ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 17, 2018

samdoran force-pushed the issue/13235-user-expires branch from 34d80e1 to b0a8917 Compare April 17, 2018 17:04

samdoran force-pushed the issue/13235-user-expires branch from b0a8917 to d27a333 Compare April 24, 2018 15:12

bcoca reviewed Apr 24, 2018

View reviewed changes

bcoca reviewed Apr 25, 2018

View reviewed changes

samdoran added 8 commits April 25, 2018 16:28

Only change expiration date if it is different

5c5fc45

Modify user_info() method to also return the password expiration. Compare current and desired expiration times and only change if they are different.

Improve formatting on user tests

9f5b0ea

Add integration test for expiration

5e56cf8

Add changelog fragment

733f9f7

Improve integration test

3bb45bf

Skip macOS and use getent module for validating expiration date.

Fix expiration change for FreeBSD

af52b74

Don't use datetime since the total_seconds method isn't available on …

bf4e3f6

…CentOS 6

Use better name for expiration index field

bfa22bb

Use separate tasks for verifying expiration date on BSD

samdoran added 2 commits April 25, 2018 16:28

Use calendar.timegm() rather than time.mktime()

2d6af93

calendar.timegm() is the inverse of time.gmtime() and returns a timestamp in UTC not localtime Add tests that change the system timezone away from UTC

Mark tests as destructive and use test for change status

7b343d2

samdoran force-pushed the issue/13235-user-expires branch from a831292 to 7b343d2 Compare April 25, 2018 20:32

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Apr 26, 2018

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 30, 2018

samdoran mentioned this pull request Apr 30, 2018

User module - expires option always fires changed #13235

Closed

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 30, 2018

bcoca merged commit 5a6bdef into ansible:devel May 1, 2018

samdoran deleted the issue/13235-user-expires branch May 1, 2018 16:06

samdoran mentioned this pull request May 1, 2018

fixes bug on user expiration always changing #25367

Closed

samdoran mentioned this pull request May 1, 2018

Backport #38885: Only change expiration date if it is different #39575

Merged

samdoran mentioned this pull request Jun 18, 2018

Improved and fixed the expiration options in system/user module #20601

Closed

ansible locked and limited conversation to collaborators May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only change expiration date if it is different #38885

Only change expiration date if it is different #38885

samdoran commented Apr 17, 2018

ansibot commented Apr 17, 2018

ansibot commented Apr 17, 2018

webknjaz commented Apr 17, 2018

ansibot commented Apr 17, 2018

bcoca commented Apr 20, 2018 •

edited by samdoran

samdoran commented Apr 24, 2018

bcoca Apr 24, 2018

samdoran Apr 25, 2018

bcoca Apr 25, 2018

samdoran commented Apr 25, 2018

mattclay commented Apr 26, 2018

samdoran commented Apr 30, 2018

Only change expiration date if it is different #38885

Only change expiration date if it is different #38885

Conversation

samdoran commented Apr 17, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Apr 17, 2018

ansibot commented Apr 17, 2018

webknjaz commented Apr 17, 2018

ansibot commented Apr 17, 2018

bcoca commented Apr 20, 2018 • edited by samdoran

samdoran commented Apr 24, 2018

bcoca Apr 24, 2018

Choose a reason for hiding this comment

samdoran Apr 25, 2018

Choose a reason for hiding this comment

bcoca Apr 25, 2018

Choose a reason for hiding this comment

samdoran commented Apr 25, 2018

mattclay commented Apr 26, 2018

samdoran commented Apr 30, 2018

bcoca commented Apr 20, 2018 •

edited by samdoran