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

Enable / Disable user from web application #27333

Merged
merged 17 commits into from Mar 21, 2017

Conversation

Projects
None yet
5 participants
@AlexLaroche
Contributor

AlexLaroche commented Mar 8, 2017

Description

The option introduce permet to enable / disable to users management in the web application.

The option is available only to an admin or a subadmin.

Related Issue

#809
#12601

Motivation and Context

Easy management of the users.

How Has This Been Tested?

The functionnality have been test with the owncloud version 9.1.4.2 installed on Ubuntu 16.04.2 LTS.

Server version: Apache/2.4.18 (Ubuntu)
Server built: 2016-07-14T12:32:26
PHP 7.0.15-0ubuntu0.16.04.4

Screenshots (if appropriate):

enabled

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 8, 2017

@AlexLaroche, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @ringmaster and @PVince81 to be potential reviewers.

@AlexLaroche, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @ringmaster and @PVince81 to be potential reviewers.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Mar 8, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Mar 8, 2017

CLA assistant check
All committers have signed the CLA.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 8, 2017

Member

Thanks a lot! Please have a look at the failing unit tests. And also please get rid of that ajax/set enabled.php

Member

DeepDiver1975 commented Mar 8, 2017

Thanks a lot! Please have a look at the failing unit tests. And also please get rid of that ajax/set enabled.php

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 8, 2017

Member

And also please get rid of that ajax/set enabled.php

which means: please use the UsersController class instead.
Basically add a new method there, you can see one here for example: https://github.com/owncloud/core/blob/master/settings/Controller/UsersController.php#L621
Then put a matching route here: https://github.com/owncloud/core/blob/master/settings/routes.php#L50

Let us know if you need help 😄

Member

PVince81 commented Mar 8, 2017

And also please get rid of that ajax/set enabled.php

which means: please use the UsersController class instead.
Basically add a new method there, you can see one here for example: https://github.com/owncloud/core/blob/master/settings/Controller/UsersController.php#L621
Then put a matching route here: https://github.com/owncloud/core/blob/master/settings/routes.php#L50

Let us know if you need help 😄

@PVince81 PVince81 added this to the 10.0 milestone Mar 8, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 8, 2017

Member

@AlexLaroche 13 unit tests are failing - please have a look - in case you need guidance just let us know. THX

https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-27333/2/testReport/

Member

DeepDiver1975 commented Mar 8, 2017

@AlexLaroche 13 unit tests are failing - please have a look - in case you need guidance just let us know. THX

https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-27333/2/testReport/

AlexLaroche added some commits Mar 8, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 9, 2017

Member

Nice! Great job!

Member

DeepDiver1975 commented Mar 9, 2017

Nice! Great job!

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 9, 2017

Member

@AlexLaroche tests passed, great. Please let us know when you think this is ready for another review. (you might still be tweaking stuff, we can't guess 😄)

Member

PVince81 commented Mar 9, 2017

@AlexLaroche tests passed, great. Please let us know when you think this is ready for another review. (you might still be tweaking stuff, we can't guess 😄)

@AlexLaroche

This comment has been minimized.

Show comment
Hide comment
@AlexLaroche

AlexLaroche Mar 9, 2017

Contributor

It's ready for review. :)

Contributor

AlexLaroche commented Mar 9, 2017

It's ready for review. :)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 9, 2017

Member
  • TODO: prevent the current user to disable themselves (on API side and also remove the checkbox on frontend)
  • TODO: would be good to also have the "undo" feature. Basically don't perform the action before 7 seconds have expired. See how delete is done. Because disabling a user kills all their sessions which is a bit nasty.
  • TODO: add display name of the user in the notification: User "{someUser}" has been disabled (not "have been" which is plural)
  • I wonder if we should hide this column by default to avoid cluttering the UI like other fields (optional)

Other than that this seems to work nicely!

I'll have a look at the code.

Member

PVince81 commented Mar 9, 2017

  • TODO: prevent the current user to disable themselves (on API side and also remove the checkbox on frontend)
  • TODO: would be good to also have the "undo" feature. Basically don't perform the action before 7 seconds have expired. See how delete is done. Because disabling a user kills all their sessions which is a bit nasty.
  • TODO: add display name of the user in the notification: User "{someUser}" has been disabled (not "have been" which is plural)
  • I wonder if we should hide this column by default to avoid cluttering the UI like other fields (optional)

Other than that this seems to work nicely!

I'll have a look at the code.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 9, 2017

Member
  • BUG: as subadmin I can disable users not in my group. I see that you put in checks in the code but they don't seem to work.

Steps to reproduce:

  1. Create three users "user1", "user2", "user3"
  2. Add "user1" and "user2" into a group "group1"
  3. Make "user1" a subadmin of group "group1"
  4. Login as "user1"
  5. Open the browser network console
  6. Disable "user2"
  7. Select the "enabled" request in the network console and do "copy as curl"
  8. Paste the curl command in a terminal and replace "user2" with "user3" and run the command

Expected: 403 error
Actual: user3 got disabled

Member

PVince81 commented Mar 9, 2017

  • BUG: as subadmin I can disable users not in my group. I see that you put in checks in the code but they don't seem to work.

Steps to reproduce:

  1. Create three users "user1", "user2", "user3"
  2. Add "user1" and "user2" into a group "group1"
  3. Make "user1" a subadmin of group "group1"
  4. Login as "user1"
  5. Open the browser network console
  6. Disable "user2"
  7. Select the "enabled" request in the network console and do "copy as curl"
  8. Paste the curl command in a terminal and replace "user2" with "user3" and run the command

Expected: 403 error
Actual: user3 got disabled

@PVince81

Apart from some minor issues (and the bugs), the code looks good!

Show outdated Hide outdated settings/Controller/UsersController.php
$userId = $this->userSession->getUser()->getUID();
$user = $this->userManager->get($id);
if($userId === $id

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

yes, prevent disabling yourself, but it seems this doesn't work correctly

@PVince81

PVince81 Mar 9, 2017

Member

yes, prevent disabling yourself, but it seems this doesn't work correctly

Show outdated Hide outdated settings/Controller/UsersController.php
);
}
if($enabled !== 'true' && $enabled !== 'false')

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

Maybe convert this to a boolean directly at the beginning of the function ?
Booleans compared as strings usually give me the shivers 😄

@PVince81

PVince81 Mar 9, 2017

Member

Maybe convert this to a boolean directly at the beginning of the function ?
Booleans compared as strings usually give me the shivers 😄

Show outdated Hide outdated settings/js/users/users.js
onEnabledChange: function() {
var $select = $(this);
var uid = UserList.getUID($select);
var enabled = $select.attr('checked') ? 'true' : 'false';

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

use the boolean $select.prop('checked') for better browser compatibility

@PVince81

PVince81 Mar 9, 2017

Member

use the boolean $select.prop('checked') for better browser compatibility

Show outdated Hide outdated settings/js/users/users.js
var enabled = $select.attr('checked') ? 'true' : 'false';
UserList._updateEnabled(uid, enabled, function(returnedEnabled){
if (enabled !== returnedEnabled) {
$select.find('#isEnabled').attr("checked", user.isEnabled);

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

use $select.prop('checked', user.isEnabled) for better browser compatibility

@PVince81

PVince81 Mar 9, 2017

Member

use $select.prop('checked', user.isEnabled) for better browser compatibility

Show outdated Hide outdated settings/js/users/users.js
function (result) {
if(result.status == 'success') {
var msg = 'User have been ' + (result.data.enabled === 'true' ? 'enabled' : 'disabled') + '!';
OC.Notification.showTemporary(t('admin', msg));

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

Please add the message directly inside the t() block instead of using a variable.
This is because the translation bot looks for the t() functions to find what text to submit for translation.

Also use a placeholder for the state: t('admin', 'User {displayName} has been {state}', {displayName: ..., state: stateText). and for the stateText: t('admin', 'enabled'), etc

@PVince81

PVince81 Mar 9, 2017

Member

Please add the message directly inside the t() block instead of using a variable.
This is because the translation bot looks for the t() functions to find what text to submit for translation.

Also use a placeholder for the state: t('admin', 'User {displayName} has been {state}', {displayName: ..., state: stateText). and for the stateText: t('admin', 'enabled'), etc

Show outdated Hide outdated settings/templates/users/part.userlist.php
@@ -46,6 +47,9 @@
><span class="title groupsList"></span><span class="icon-triangle-s"></span></div>
</td>
<?php endif;?>
<td class="enabled">
<input type="checkbox" id="isEnabled" checked="checked">

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

Please use a class instead of id because ids must be unique within the page but here it would be repeated across the page. This will also require changing your jQuery selectors.

@PVince81

PVince81 Mar 9, 2017

Member

Please use a class instead of id because ids must be unique within the page but here it would be repeated across the page. This will also require changing your jQuery selectors.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 17, 2017

Member

@AlexLaroche ready for review again or are you still working on it ? (please always post a message when ready for review)

Member

PVince81 commented Mar 17, 2017

@AlexLaroche ready for review again or are you still working on it ? (please always post a message when ready for review)

@AlexLaroche

This comment has been minimized.

Show comment
Hide comment
@AlexLaroche

AlexLaroche Mar 17, 2017

Contributor

@PVince81 : In your requirements to merge the code, it still missing :

  • TODO: would be good to also have the "undo" feature. Basically don't perform the action before 7 seconds have expired. See how delete is done. Because disabling a user kills all their sessions which is a bit nasty.

I have don't time to add it for the moment. I will try to implement it when I will have more time.
It why a don't have ask for the moment for an another review.

Contributor

AlexLaroche commented Mar 17, 2017

@PVince81 : In your requirements to merge the code, it still missing :

  • TODO: would be good to also have the "undo" feature. Basically don't perform the action before 7 seconds have expired. See how delete is done. Because disabling a user kills all their sessions which is a bit nasty.

I have don't time to add it for the moment. I will try to implement it when I will have more time.
It why a don't have ask for the moment for an another review.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 17, 2017

Member

I think the undo thing can be implemented separately.

Let me rereview.

Member

PVince81 commented Mar 17, 2017

I think the undo thing can be implemented separately.

Let me rereview.

Show outdated Hide outdated settings/js/users/users.js
@@ -25,8 +26,11 @@ var UserList = {
initialize: function($el) {
this.$el = $el;
UserList.currentUser = document.getElementsByTagName('head')[0].getAttribute('data-user');

This comment has been minimized.

@PVince81

PVince81 Mar 17, 2017

Member

you can use OC.getCurrentUser().uid which would be slightly better

@PVince81

PVince81 Mar 17, 2017

Member

you can use OC.getCurrentUser().uid which would be slightly better

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 17, 2017

Member
  • BUG: even though the checkbox "show enabled column" is disabled, if you refresh the page the column still appears. I couldn't spot the bug in the code after a quick look, maybe needs some debugging.

I've retested and apart from these two points I think this is good to go.

Member

PVince81 commented Mar 17, 2017

  • BUG: even though the checkbox "show enabled column" is disabled, if you refresh the page the column still appears. I couldn't spot the bug in the code after a quick look, maybe needs some debugging.

I've retested and apart from these two points I think this is good to go.

AlexLaroche added some commits Mar 17, 2017

@AlexLaroche

This comment has been minimized.

Show comment
Hide comment
@AlexLaroche

AlexLaroche Mar 20, 2017

Contributor

@PVince81 : All should be OK now. It was a single line of code that cause the bug. :)

Contributor

AlexLaroche commented Mar 20, 2017

@PVince81 : All should be OK now. It was a single line of code that cause the bug. :)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

Tested, works 👍
Great work, thanks a lot!

Member

PVince81 commented Mar 21, 2017

Tested, works 👍
Great work, thanks a lot!

@PVince81 PVince81 merged commit 53c0129 into owncloud:master Mar 21, 2017

4 checks passed

Scrutinizer 4 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment