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

Add occ commands to enable and disable a user + a disabled user can n… #23844

Merged
merged 6 commits into from
May 3, 2016
Merged

Add occ commands to enable and disable a user + a disabled user can n… #23844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c9fec3
Add occ commands to enable and disable a user + a disabled user can n…
DeepDiver1975 Apr 7, 2016
8486926
Add provisioning api to enable and disable users
DeepDiver1975 Apr 7, 2016
0eed6b5
Integration test for a disabled user
DeepDiver1975 Apr 8, 2016
271ba6d
Added integration tests directly in the code PR
SergioBertolinSG Apr 19, 2016
9b875db
OCS API should catch LoginExceptions
rullzer Apr 20, 2016
661ab1a
Modified expected output according to rullzer
SergioBertolinSG Apr 20, 2016
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
11 changes: 8 additions & 3 deletions apps/provisioning_api/appinfo/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,13 @@

namespace OCA\Provisioning_API\AppInfo;

use OCA\Provisioning_API\Apps;
use OCA\Provisioning_API\Groups;
use OCA\Provisioning_API\Users;
use OCP\API;

// Users
$users = new \OCA\Provisioning_API\Users(
$users = new Users(
\OC::$server->getUserManager(),
\OC::$server->getConfig(),
\OC::$server->getGroupManager(),
Expand All @@ -41,6 +44,8 @@
API::register('get', '/cloud/users/{userid}', [$users, 'getUser'], 'provisioning_api', API::USER_AUTH);
API::register('put', '/cloud/users/{userid}', [$users, 'editUser'], 'provisioning_api', API::USER_AUTH);
API::register('delete', '/cloud/users/{userid}', [$users, 'deleteUser'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('put', '/cloud/users/{userid}/enable', [$users, 'enableUser'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('put', '/cloud/users/{userid}/disable', [$users, 'disableUser'], 'provisioning_api', API::SUBADMIN_AUTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general to be more RESTful another idea would be to still do a "PUT" on "/cloud/users/{userid}" and set the "enabled" attribute to true or false. (PUT being an "edit" operation)

This also means that the GET operation would return the "enabled" flag as well with the user's properties.

I'm ok with keeping in that way if that's too much to rework.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that GET already returns the flag. So intuitively as an API consumer, I'd be tempted to try and set it the way I described.

However now I see that it says "displayName" in the response but the PUT value is "display" 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Well - true restful would be to have only one GET to retrieve a json object holding all information and one PUT where we can push a full json object to store the new state.

But the whole provisioning api is not REST - so nothing to worry about - in the scope for v1.

Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 agreed

API::register('get', '/cloud/users/{userid}/groups', [$users, 'getUsersGroups'], 'provisioning_api', API::USER_AUTH);
API::register('post', '/cloud/users/{userid}/groups', [$users, 'addToGroup'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('delete', '/cloud/users/{userid}/groups', [$users, 'removeFromGroup'], 'provisioning_api', API::SUBADMIN_AUTH);
Expand All @@ -49,7 +54,7 @@
API::register('get', '/cloud/users/{userid}/subadmins', [$users, 'getUserSubAdminGroups'], 'provisioning_api', API::ADMIN_AUTH);

// Groups
$groups = new \OCA\Provisioning_API\Groups(
$groups = new Groups(
\OC::$server->getGroupManager(),
\OC::$server->getUserSession(),
\OC::$server->getRequest()
Expand All @@ -61,7 +66,7 @@
API::register('get', '/cloud/groups/{groupid}/subadmins', [$groups, 'getSubAdminsOfGroup'], 'provisioning_api', API::ADMIN_AUTH);

// Apps
$apps = new \OCA\Provisioning_API\Apps(
$apps = new Apps(
\OC::$server->getAppManager(),
\OC::$server->getOcsClient()
);
Expand Down
72 changes: 60 additions & 12 deletions apps/provisioning_api/lib/users.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,32 +31,36 @@
use \OC_OCS_Result;
use \OC_Helper;
use OCP\Files\NotFoundException;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\ILogger;
use OCP\IUserManager;
use OCP\IUserSession;

class Users {

/** @var \OCP\IUserManager */
/** @var IUserManager */
private $userManager;
/** @var \OCP\IConfig */
/** @var IConfig */
private $config;
/** @var \OCP\IGroupManager */
/** @var IGroupManager */
private $groupManager;
/** @var \OCP\IUserSession */
/** @var IUserSession */
private $userSession;
/** @var ILogger */
private $logger;

/**
* @param \OCP\IUserManager $userManager
* @param \OCP\IConfig $config
* @param \OCP\IGroupManager $groupManager
* @param \OCP\IUserSession $userSession
* @param IUserManager $userManager
* @param IConfig $config
* @param IGroupManager $groupManager
* @param IUserSession $userSession
* @param ILogger $logger
*/
public function __construct(\OCP\IUserManager $userManager,
\OCP\IConfig $config,
\OCP\IGroupManager $groupManager,
\OCP\IUserSession $userSession,
public function __construct(IUserManager $userManager,
IConfig $config,
IGroupManager $groupManager,
IUserSession $userSession,
ILogger $logger) {
$this->userManager = $userManager;
$this->config = $config;
Expand Down Expand Up @@ -329,6 +333,50 @@ public function deleteUser($parameters) {
}
}

/**
* @param array $parameters
* @return OC_OCS_Result
*/
public function disableUser($parameters) {
return $this->setEnabled($parameters, false);
}

/**
* @param array $parameters
* @return OC_OCS_Result
*/
public function enableUser($parameters) {
return $this->setEnabled($parameters, true);
}

/**
* @param array $parameters
* @param bool $value
* @return OC_OCS_Result
*/
private function setEnabled($parameters, $value) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like some integration tests on this covering the following scenarios:

  1. Subadmin should be able to enable or disable an user in their group
  2. Subadmins should NOT be able to enable or disable an user not in their group
  3. Subadmins should not be able to disable users that have admin permissions even if they are in their group
  4. Admins can enable and disable any user
  5. Regular users will get an exception
  6. It should not be possible to disable or enable ones own account, neither as admin nor as subadmin

cc @owncloud/qa

Copy link
Contributor

@nickvergessen nickvergessen Apr 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

\3. is not covered by the code?

Copy link
Member

Choose a reason for hiding this comment

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

It is. Check L370

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Bloody GitHub, if I click on edit it shows "3." instead of "1."

Yes. 3, is not covered yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SergioBertolinSG do we have this covered now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it works fine.

// Check if user is logged in
$currentLoggedInUser = $this->userSession->getUser();
if ($currentLoggedInUser === null) {
return new OC_OCS_Result(null, \OCP\API::RESPOND_UNAUTHORISED);
}

$targetUser = $this->userManager->get($parameters['userid']);
if($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) {
return new OC_OCS_Result(null, 101);
}

// If not permitted
$subAdminManager = $this->groupManager->getSubAdmin();
if(!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
return new OC_OCS_Result(null, 997);
}

// enable/disable the user now
$targetUser->setEnabled($value);
return new OC_OCS_Result(null, 100);
}

/**
* @param array $parameters
* @return OC_OCS_Result
Expand Down
64 changes: 60 additions & 4 deletions apps/provisioning_api/tests/userstest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ protected function tearDown() {
parent::tearDown();
}

protected function setup() {
parent::setup();
protected function setUp() {
parent::setUp();

$this->userManager = $this->getMock('\OCP\IUserManager');
$this->config = $this->getMock('\OCP\IConfig');
Expand Down Expand Up @@ -540,7 +540,7 @@ public function testAddUserAsSubAdminValidGroupNotSubAdmin() {
->expects($this->once())
->method('isSubAdminOfGroup')
->with($loggedInUser, $existingGroup)
->wilLReturn(false);
->willReturn(false);
$this->groupManager
->expects($this->once())
->method('getSubAdmin')
Expand Down Expand Up @@ -642,7 +642,7 @@ public function testAddUserAsSubAdminExistingGroups() {
[$loggedInUser, $existingGroup1],
[$loggedInUser, $existingGroup2]
)
->wilLReturn(true);
->willReturn(true);


$expected = new \OC_OCS_Result(null, 100);
Expand Down Expand Up @@ -2295,4 +2295,60 @@ public function testGetUserSubAdminGroupsWithoutGroups() {
$expected = new \OC_OCS_Result(null, 102, 'Unknown error occurred');
$this->assertEquals($expected, $this->api->getUserSubAdminGroups(['userid' => 'RequestedUser']));
}

public function testEnableUser() {
$targetUser = $this->getMock('\OCP\IUser');
$targetUser->expects($this->once())
->method('setEnabled')
->with(true);
$this->userManager
->expects($this->once())
->method('get')
->with('RequestedUser')
->will($this->returnValue($targetUser));
$loggedInUser = $this->getMock('\OCP\IUser');
$loggedInUser
->expects($this->exactly(2))
->method('getUID')
->will($this->returnValue('admin'));
$this->userSession
->expects($this->once())
->method('getUser')
->will($this->returnValue($loggedInUser));
$this->groupManager
->expects($this->once())
->method('isAdmin')
->will($this->returnValue(true));

$expected = new \OC_OCS_Result(null, 100);
$this->assertEquals($expected, $this->api->enableUser(['userid' => 'RequestedUser']));
}

public function testDisableUser() {
$targetUser = $this->getMock('\OCP\IUser');
$targetUser->expects($this->once())
->method('setEnabled')
->with(false);
$this->userManager
->expects($this->once())
->method('get')
->with('RequestedUser')
->will($this->returnValue($targetUser));
$loggedInUser = $this->getMock('\OCP\IUser');
$loggedInUser
->expects($this->exactly(2))
->method('getUID')
->will($this->returnValue('admin'));
$this->userSession
->expects($this->once())
->method('getUser')
->will($this->returnValue($loggedInUser));
$this->groupManager
->expects($this->once())
->method('isAdmin')
->will($this->returnValue(true));

$expected = new \OC_OCS_Result(null, 100);
$this->assertEquals($expected, $this->api->disableUser(['userid' => 'RequestedUser']));
}
}
67 changes: 66 additions & 1 deletion build/integration/features/bootstrap/Provisioning.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public function assureUserExists($user) {
}
$this->userExists($user);
PHPUnit_Framework_Assert::assertEquals(200, $this->response->getStatusCode());

}

/**
Expand Down Expand Up @@ -230,6 +229,20 @@ public function creatingTheGroup($group) {
}
}

/**
* @When /^assure user "([^"]*)" is disabled$/
*/
public function assureUserIsDisabled($user) {
$fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user/disable";
$client = new Client();
$options = [];
if ($this->currentUser === 'admin') {
$options['auth'] = $this->adminUser;
}

$this->response = $client->send($client->createRequest("PUT", $fullUrl, $options));
}

/**
* @When /^Deleting the user "([^"]*)"$/
* @param string $user
Expand Down Expand Up @@ -363,6 +376,25 @@ public function userIsSubadminOfGroup($user, $group) {
PHPUnit_Framework_Assert::assertEquals(200, $this->response->getStatusCode());
}

/**
* @Given /^Assure user "([^"]*)" is subadmin of group "([^"]*)"$/
* @param string $user
* @param string $group
*/
public function assureUserIsSubadminOfGroup($user, $group) {
$fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user/subadmins";
$client = new Client();
$options = [];
if ($this->currentUser === 'admin') {
$options['auth'] = $this->adminUser;
}
$options['body'] = [
'groupid' => $group
];
$this->response = $client->send($client->createRequest("POST", $fullUrl, $options));
PHPUnit_Framework_Assert::assertEquals(200, $this->response->getStatusCode());
}

/**
* @Given /^user "([^"]*)" is not a subadmin of group "([^"]*)"$/
* @param string $user
Expand Down Expand Up @@ -528,6 +560,38 @@ public function appIsEnabled($app) {
PHPUnit_Framework_Assert::assertEquals(200, $this->response->getStatusCode());
}

/**
* @Then /^user "([^"]*)" is disabled$/
* @param string $user
*/
public function userIsDisabled($user) {
$fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user";
$client = new Client();
$options = [];
if ($this->currentUser === 'admin') {
$options['auth'] = $this->adminUser;
}

$this->response = $client->get($fullUrl, $options);
PHPUnit_Framework_Assert::assertEquals("false", $this->response->xml()->data[0]->enabled);
}

/**
* @Then /^user "([^"]*)" is enabled$/
* @param string $user
*/
public function userIsEnabled($user) {
$fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user";
$client = new Client();
$options = [];
if ($this->currentUser === 'admin') {
$options['auth'] = $this->adminUser;
}

$this->response = $client->get($fullUrl, $options);
PHPUnit_Framework_Assert::assertEquals("true", $this->response->xml()->data[0]->enabled);
}

/**
* @Given user :user has a quota of :quota
* @param string $user
Expand Down Expand Up @@ -588,4 +652,5 @@ public function cleanupGroups()
}
$this->usingServer($previousServer);
}

}
10 changes: 10 additions & 0 deletions build/integration/features/bootstrap/WebDav.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,5 +424,15 @@ public function userMovesNewChunkFileWithIdToMychunkedfile($user, $id, $dest)
}


/**
* @Given /^Downloading file "([^"]*)" as "([^"]*)"$/
*/
public function downloadingFileAs($fileName, $user) {
try {
$this->response = $this->makeDavRequest($user, 'GET', $fileName, []);
} catch (\GuzzleHttp\Exception\ServerException $ex) {
$this->response = $ex->getResponse();
}
}
}

Loading