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.

Sign up for GitHub

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

Fix processing of multiple groups #144

Merged
merged 4 commits into from
Sep 28, 2018
+236 −6
Merged

Fix processing of multiple groups #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d39c23
Do not show invalid argument when there is a group given
phil-davis Sep 28, 2018
6583125
Do not exit early if a missing group is found
phil-davis Sep 28, 2018
8ac1a71
Continue processing all groups and UIDs and report problems at the end
phil-davis Sep 28, 2018
a470435
Unit test update to fix processing of multiple groups
sharidas Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions lib/Command/ExpirePassword.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,10 @@ protected function execute(InputInterface $input, OutputInterface $output) {
}
});
} else {
if (\count($groups) >= 1) {
$numGroupsMissing = 0;
$numUidsMissing = 0;

if (\count($groups) > 0) {
foreach ($groups as $group) {
if ($this->groupManager->groupExists($group) === true) {
foreach ($this->groupManager->findUsersInGroup($group) as $user) {
Expand All @@ -169,8 +172,8 @@ protected function execute(InputInterface $input, OutputInterface $output) {
}
}
} else {
$output->writeln("Ignoring missing group $group");
return 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PVince81 what should happen here?

Since we may have already processed some groups, I think we should continue on and process any other groups.

But then after looping through all the groups, should we remember that there was a problem with some group, and exit with non-zero status?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, sounds good. continue and report at the end

Copy link
Contributor

Choose a reason for hiding this comment

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

fix now or create ticket for later for the additional reporting ?

$output->writeln("<error>Unknown group: $group</error>");
$numGroupsMissing = $numGroupsMissing + 1;
}
}
}
Expand All @@ -182,7 +185,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {

if ($user === null) {
$output->writeln("<error>Unknown user: $uid</error>");
return 2;
$numUidsMissing = $numUidsMissing + 1;
} else {
if (isset($users[$user->getUID()])) {
continue;
Expand All @@ -196,8 +199,31 @@ protected function execute(InputInterface $input, OutputInterface $output) {
}
}
}
} else {
}

if (!(\count($groups) > 0) && !(\count($uids) > 0)) {
$output->writeln("<error>Invalid argument given.</error>");
return 1;
}

if (($numGroupsMissing > 0) || ($numUidsMissing > 0)) {
if ($numGroupsMissing > 0) {
if ($numGroupsMissing > 1) {
$text = "$numGroupsMissing groups were not known";
} else {
$text = "1 group was not known";
}
$output->writeln("<error>$text</error>");
}
if ($numUidsMissing > 0) {
if ($numUidsMissing > 1) {
$text = "$numUidsMissing users were not known";
} else {
$text = "1 user was not known";
}
$output->writeln("<error>$text</error>");
}
return 2;
}
}
}
Expand Down
206 changes: 205 additions & 1 deletion tests/unit/Command/ExpirePasswordTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ public function testGroupsExpirePassword() {
$expectedOutput = "The password for user1 is set to expire on 2019-01-01 14:00:00 UTC.
The password for user2 is set to expire on 2019-01-01 14:00:00 UTC.
The password for user3 is set to expire on 2019-01-01 14:00:00 UTC.
Ignoring missing group group3
Unknown group: group3
1 group was not known
";
$this->assertEquals($expectedOutput, $output);
}
Expand Down Expand Up @@ -309,4 +310,207 @@ public function testInvalidArgument() {

$this->assertContains('Invalid argument given.', $output);
}

public function testNonExistingGroups() {
$this->timeFactory
->method('getTime')
->willReturn(1530180780);

$this->config
->method('getAppValue')
->will($this->returnValueMap([
['password_policy', 'spv_user_password_expiration_checked', false, 'on'],
['password_policy', 'spv_user_password_expiration_value', 90, 10]
]));

$this->commandTester->execute([
'--group' => ['group1', 'group2']
]);
$output = $this->commandTester->getDisplay();
$expectedOutput = "Unknown group: group1
Unknown group: group2
2 groups were not known
";
$this->assertEquals($expectedOutput, $output);
}

public function testNonExistingUsers() {
$this->timeFactory
->method('getTime')
->willReturn(1530180780);

$this->config
->method('getAppValue')
->will($this->returnValueMap([
['password_policy', 'spv_user_password_expiration_checked', false, 'on'],
['password_policy', 'spv_user_password_expiration_value', 90, 10]
]));

$this->commandTester->execute([
'--uid' => ['user1', 'user2']
]);
$output = $this->commandTester->getDisplay();
$expectedOutput = "Unknown user: user1
Unknown user: user2
2 users were not known
";
$this->assertEquals($expectedOutput, $output);
}

public function testExistingUsersAndGroups() {
$this->timeFactory
->method('getTime')
->willReturn(1530180780);

$this->config
->method('getAppValue')
->will($this->returnValueMap([
['password_policy', 'spv_user_password_expiration_checked', false, 'on'],
['password_policy', 'spv_user_password_expiration_value', 90, 10]
]));

$user1 = $this->createMock(IUser::class);
$user1->method('getUID')
->willReturn('user1');
$user1->method('canChangePassword')
->willReturn(true);
$user2 = $this->createMock(IUser::class);
$user2->method('getUID')
->willReturn('user2');
$user2->method('canChangePassword')
->willReturn(true);
$user3 = $this->createMock(IUser::class);
$user3->method('getUID')
->willReturn('user3');
$user3->method('canChangePassword')
->willReturn(true);
$user4 = $this->createMock(IUser::class);
$user4->method('getUID')
->willReturn('user4');
$user4->method('canChangePassword')
->willReturn(true);
$group1Users = [$user1, $user2];
$group2Users = [$user3, $user4];

$this->groupManager
->method('groupExists')
->will($this->returnValueMap([
['group1', true],
['group2', true]
]));
$this->groupManager
->method('findUsersInGroup')
->willReturnOnConsecutiveCalls($group1Users, $group2Users);
$this->userManager
->method('get')
->willReturnOnConsecutiveCalls($user1, $user2, $user3, $user4);

$this->commandTester->execute([
'--uid' => ['user1', 'user2', 'user3', 'user4'],
'--group' => ['group1', 'group2']

]);
$output = $this->commandTester->getDisplay();
$expectedOutput = "The password for user1 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user2 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user3 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user4 is set to expire on 2018-06-27 10:13:00 UTC.
";
$this->assertEquals($expectedOutput, $output);
}

public function testExistingUsersAndGroupsSomeDontExist() {
$this->timeFactory
->method('getTime')
->willReturn(1530180780);

$this->config
->method('getAppValue')
->will($this->returnValueMap([
['password_policy', 'spv_user_password_expiration_checked', false, 'on'],
['password_policy', 'spv_user_password_expiration_value', 90, 10]
]));

$user1 = $this->createMock(IUser::class);
$user1->method('getUID')
->willReturn('user1');
$user1->method('canChangePassword')
->willReturn(true);
$user2 = $this->createMock(IUser::class);
$user2->method('getUID')
->willReturn('user2');
$user2->method('canChangePassword')
->willReturn(true);
$user3 = $this->createMock(IUser::class);
$user3->method('getUID')
->willReturn('user3');
$user3->method('canChangePassword')
->willReturn(true);
$user4 = $this->createMock(IUser::class);
$user4->method('getUID')
->willReturn('user4');
$user4->method('canChangePassword')
->willReturn(true);
$user5 = $this->createMock(IUser::class);
$user5->method('getUID')
->willReturn('user5');
$user5->method('canChangePassword')
->willReturn(true);
$user6 = $this->createMock(IUser::class);
$user6->method('getUID')
->willReturn('user6');
$user6->method('canChangePassword')
->willReturn(true);
$user7 = $this->createMock(IUser::class);
$user7->method('getUID')
->willReturn('user7');
$user7->method('canChangePassword')
->willReturn(true);
$user8 = $this->createMock(IUser::class);
$user8->method('getUID')
->willReturn('user8');
$user8->method('canChangePassword')
->willReturn(true);

$group1Users = [$user1, $user2];
$group2Users = [$user3, $user4];
$group3Users = [$user5, $user6];
$group4Users = [$user7, $user8];

$this->groupManager
->method('groupExists')
->will($this->returnValueMap([
['group1', true],
['group2', true],
['group3', false],
['group4', false]
]));
$this->groupManager
->method('findUsersInGroup')
->willReturnOnConsecutiveCalls($group1Users, $group2Users, $group3Users, $group4Users);
$this->userManager
->method('get')
->willReturnOnConsecutiveCalls($user1, $user2, $user3, $user4);

$this->commandTester->execute([
'--uid' => ['user1', 'user2', 'user3', 'user4', 'user5', 'user6', 'user7', 'user8'],
'--group' => ['group1', 'group2', 'group3', 'group4']

]);
$output = $this->commandTester->getDisplay();
$expectedOutput = "The password for user1 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user2 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user3 is set to expire on 2018-06-27 10:13:00 UTC.
The password for user4 is set to expire on 2018-06-27 10:13:00 UTC.
Unknown group: group3
Unknown group: group4
Unknown user: user5
Unknown user: user6
Unknown user: user7
Unknown user: user8
2 groups were not known
4 users were not known
";
$this->assertEquals($expectedOutput, $output);
}
}