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

DeepDiver1975 · 2016-04-07T14:46:55Z

…o longer login - fixes #23838

mention-bot · 2016-04-07T14:46:56Z

By analyzing the blame information on this pull request, we identified @icewind1991, @butonic and @xoen to be potential reviewers

LukasReschke · 2016-04-07T16:25:49Z

apps/provisioning_api/lib/users.php

+	 * @param bool $value
+	 * @return OC_OCS_Result
+	 */
+	private function setEnabled($parameters, $value) {


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

Subadmin should be able to enable or disable an user in their group

Subadmins should NOT be able to enable or disable an user not in their group

Subadmins should not be able to disable users that have admin permissions even if they are in their group

Admins can enable and disable any user

Regular users will get an exception

It should not be possible to disable or enable ones own account, neither as admin nor as subadmin

cc @owncloud/qa

\3. is not covered by the code?

It is. Check L370

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

Yes. 3, is not covered yet.

@SergioBertolinSG do we have this covered now ?

Yes it works fine.

DeepDiver1975 · 2016-04-07T17:41:45Z

Additional integration tests to be implemented:

disable user
503 is received via dav
503 is received via ocs api calls
503 is received via web requests

rullzer · 2016-04-07T18:15:32Z

I did not check yet. But does this somehow yield the users shares invalid?

LukasReschke · 2016-04-07T19:10:47Z

I did not check yet. But does this somehow yield the users shares invalid?

~~Is this really required? I mean if you disable an user on an Active Directory server one can also still access files that belonged to them~~ 🙈

Edit: Ah… Right… The migration scenario, my bad :)

butonic · 2016-04-08T10:21:26Z

lib/private/user/session.php

@@ -229,7 +235,7 @@ public function login($uid, $password) {
 						throw new LoginException('Login canceled by app');
 					}
 				} else {
-					return false;
+					throw new LoginException('User disabled');


Where do we localize the string so the user can be shown a proper message when he tries to login?

Should be done here - thanks for the pointer

MorrisJobke · 2016-04-11T08:46:35Z

build/integration/features/webdav-related.feature

@@ -241,3 +241,10 @@ Feature: webdav-related
 			| 0 |
 			| 1 |
 			| 3 |
+
+		Scenario: A disabled user cannot use webdav


This seems to fail 🙈

DeepDiver1975 · 2016-04-12T09:05:40Z

😠

Checked 4387 files in 45.2 seconds
Syntax error found in 1 file
------------------------------------------------------------
Parse error: ./lib/composer/composer/autoload_static.php:22
    20|         'OC\\Settings\\' => 
    21|         array (
  > 22|             0 => __DIR__ . '/../../..' . '/settings',
    23|         ),
    24|         'OC\\Core\\' => Unexpected '.', expecting ')'

DeepDiver1975 · 2016-04-12T09:06:13Z

@nickvergessen @LukasReschke @MorrisJobke any idea why in this pr the composer generated file is causing trouble?

DeepDiver1975 · 2016-04-12T09:06:55Z

issues seem to be related to php 5.4 and 5.5

DeepDiver1975 · 2016-04-12T09:07:35Z

same fun in #23708 - master broken?

LukasReschke · 2016-04-12T09:13:52Z

Mhm. We don't ship any /lib/composer/composer/autoload_static.php, seems like composer on Travis generates this automatically… Let's see why it does that 😉

LukasReschke · 2016-04-12T09:17:47Z

Mhm…

Composer version 1.1-dev (4f0f8779cb6db67d2186e3480f5ca2243e9916e5) 2016-04-11 18:36:39

Caused by composer/composer@fd2f51cea8e5f1ef978cd8f90b87b69dc1778976… Let me see what one can do about that …

LukasReschke · 2016-04-12T09:25:21Z

Ok. autoload_static.php is only loaded when PHP >= 5.6 is used. #23935 adjusts the linter accordingly.

DeepDiver1975 · 2016-04-13T11:24:26Z

@MorrisJobke @LukasReschke @rullzer please review and test once more - basic integration test added - needs further work @SergioBertolinSG

THX

nickvergessen · 2016-04-13T11:47:14Z

apps/provisioning_api/tests/userstest.php

@@ -59,7 +59,7 @@ protected function tearDown() {
 	}

 	protected function setup() {


camelCase missing as well

nickvergessen · 2016-04-13T11:51:16Z

Btw in terms of tracking, would be nice to have a hook for the admin_audit app

DeepDiver1975 · 2016-04-13T12:06:27Z

Btw in terms of tracking, would be nice to have a hook for the admin_audit app

agreed - please open a ticket in core and enterprise so that we can consume it. THX

PVince81 · 2016-04-29T15:16:17Z

Integration tests failing:

06:19:13     /ssd/jenkins/workspace/ocs-api-integration-tests-ci@3/build/integration/features/provisioning-v1.feature:503
06:19:13     /ssd/jenkins/workspace/ocs-api-integration-tests-ci@3/build/integration/features/sharing-v1.feature:700

(log is too big)

DeepDiver1975 · 2016-04-29T15:32:28Z

Oh shit ... I guess I fucked up the rebase .....

…o longer login - fixes #23838

Catching the login exception and returning false (login failed). Makes the OCS API properly return data instead of printing the exception page.

PVince81 · 2016-05-03T08:46:15Z

So much bad luck. Now that I was determined to review and test this, LDAP login is broken on this branch and master also... #24409

PVince81 · 2016-05-03T09:13:36Z

DeepDiver1975 · 2016-05-03T09:15:31Z

Clear cookies
Login as "user1" (when enabled)
On the CLI, disable "user1"
In the web UI as "user1", click "Log out"
Login as "admin" again => "CSRF check failed" page.

@LukasReschke mind taking a look? THX

PVince81 · 2016-05-03T09:19:15Z

apps/provisioning_api/appinfo/routes.php

@@ -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);


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.

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" 😞

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.

🙈 agreed

PVince81 · 2016-05-03T09:55:38Z

Looks good from my POV 👍

@DeepDiver1975 did you test this with shib ?

DeepDiver1975 · 2016-05-03T10:00:46Z

@DeepDiver1975 did you test this with shib ?

no - I have currently no working shib test environment at hand - I suggest to merge and test this by QA

davitol · 2016-05-03T11:58:48Z

Testing with Shib and Autoprovisioning:

Disabling/Enabling a user seems to work fine. But when disabling a shibboleth user and trying to log in with him, the following page is shown. Not sure if it is intended or not:

No logs are written in owncloud.log

Tested using Testshib and autoprovisioning mode

Steps tested:

Apply the patch
Disable a Shib user
Try to log in with this user
Enable the same Shib user
Log in with this user

DeepDiver1975 · 2016-05-03T12:16:00Z

I guess there is not much more we can do from a core pov. I guess we need to capture this situation and display a page like: you are disabled.

This has to be done in the shibboleth app

PVince81 · 2016-05-03T12:21:41Z

"you are disabled" sounds a bit funny 😄

davitol · 2016-05-03T13:22:00Z

👍 in that case

DeepDiver1975 · 2016-05-03T13:23:09Z

@davitol please open an issue in the shibboleth repo - THX

lock · 2019-08-06T03:01:28Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added the 3 - To Review label Apr 7, 2016

DeepDiver1975 added this to the 9.1-current milestone Apr 7, 2016

DeepDiver1975 added the in progress label Apr 7, 2016

LukasReschke reviewed Apr 7, 2016
View reviewed changes

butonic reviewed Apr 8, 2016
View reviewed changes

DeepDiver1975 force-pushed the disable-user branch from 0235486 to e9d6087 Compare April 8, 2016 14:32

MorrisJobke reviewed Apr 11, 2016
View reviewed changes

DeepDiver1975 force-pushed the disable-user branch 3 times, most recently from 9124c18 to 2a94fd2 Compare April 12, 2016 07:34

DeepDiver1975 force-pushed the disable-user branch from 2a94fd2 to a5d71fa Compare April 12, 2016 10:08

nickvergessen reviewed Apr 13, 2016
View reviewed changes

nickvergessen mentioned this pull request Apr 13, 2016

Dispatch an event when enabling/disabling a user #23970

Closed

DeepDiver1975 force-pushed the disable-user branch 2 times, most recently from 2247d33 to 29c3fd1 Compare April 14, 2016 15:46

SergioBertolinSG mentioned this pull request Apr 18, 2016

Add integration tests for enabling and disabling users. owncloud/QA#201

Closed

10 tasks

DeepDiver1975 force-pushed the disable-user branch from 8514ac9 to 68cb185 Compare April 29, 2016 15:38

DeepDiver1975 and others added 6 commits May 2, 2016 09:31

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

9c9fec3

…o longer login - fixes #23838

Add provisioning api to enable and disable users

8486926

Integration test for a disabled user

0eed6b5

Added integration tests directly in the code PR

271ba6d

OCS API should catch LoginExceptions

9b875db

Catching the login exception and returning false (login failed). Makes the OCS API properly return data instead of printing the exception page.

Modified expected output according to rullzer

661ab1a

DeepDiver1975 force-pushed the disable-user branch from 68cb185 to 661ab1a Compare May 2, 2016 07:31

PVince81 reviewed May 3, 2016
View reviewed changes

davitol added the tested label May 3, 2016

DeepDiver1975 merged commit 4b25449 into master May 3, 2016

DeepDiver1975 deleted the disable-user branch May 3, 2016 13:22

DeepDiver1975 added 4 - To release and removed 3 - To Review labels May 3, 2016

PVince81 mentioned this pull request May 9, 2016

add "disable user" feature #24501

Closed

PVince81 mentioned this pull request Jul 5, 2018

Enable/Disable a user documentation missing in User Provisioning API owncloud-archive/documentation#4079

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019

		@@ -59,7 +59,7 @@ protected function tearDown() {
		}

		protected function setup() {

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

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

Conversation

DeepDiver1975 commented Apr 7, 2016

mention-bot commented Apr 7, 2016

Choose a reason for hiding this comment

nickvergessen Apr 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Apr 7, 2016

rullzer commented Apr 7, 2016

LukasReschke commented Apr 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Apr 12, 2016

DeepDiver1975 commented Apr 12, 2016

DeepDiver1975 commented Apr 12, 2016

DeepDiver1975 commented Apr 12, 2016

LukasReschke commented Apr 12, 2016

LukasReschke commented Apr 12, 2016

LukasReschke commented Apr 12, 2016

DeepDiver1975 commented Apr 13, 2016

Choose a reason for hiding this comment

nickvergessen commented Apr 13, 2016

DeepDiver1975 commented Apr 13, 2016

PVince81 commented Apr 29, 2016

DeepDiver1975 commented Apr 29, 2016

PVince81 commented May 3, 2016

PVince81 commented May 3, 2016 • edited Loading

DeepDiver1975 commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented May 3, 2016

DeepDiver1975 commented May 3, 2016

davitol commented May 3, 2016 • edited Loading

DeepDiver1975 commented May 3, 2016

PVince81 commented May 3, 2016

davitol commented May 3, 2016

DeepDiver1975 commented May 3, 2016

lock bot commented Aug 6, 2019

nickvergessen Apr 13, 2016 •

edited

Loading

PVince81 commented May 3, 2016 •

edited

Loading

davitol commented May 3, 2016 •

edited

Loading