Improve impersonate app with changes #16

sharidas · 2017-04-12T05:57:34Z

Below are the changes implemented:

Support of groups which can be impersonated.
Support of logout which switches back to orignal user.
Support to display the user logged as information as notification.
Removed the confirmation dialog to impersonate.

Signed-off-by: Sujith H sharidasan@owncloud.com

CLAassistant · 2017-04-12T05:57:39Z

All committers have signed the CLA.

sharidas · 2017-04-12T06:14:26Z

@butonic @PVince81 Kindly review my changes.

PVince81 · 2017-04-12T07:38:51Z

appinfo/app.php

+);
+$eventDispatcher->addListener(
+	'OC\TemplateLayout::loadAdditionalScripts',


I grepped the core code but didn't find this hook. How can it work ?

This should help: https://github.com/owncloud/core/pull/27629/files#diff-7afe0377257ef7362517b1e27bda7ee6R66

PVince81 · 2017-04-12T07:41:31Z

controller/logoutcontroller.php

+     *  @return JSONResponse
+     */
+    public function logoutcontroller($userid) {
+        $user = $this->userManager->get($_SESSION['oldUserId']);


Would be better to use OC::$server->getSession()->get('oldUserId'), did you try it ?
Normally that's the way to get/set session values.

However from what I remember the session values are encrypted somehow with the user id and other user data. So since we are switching users the encryption could break.

Please try it out still.

and here I mean use this approach everywhere instead of $_SESSION, not just on this line

PVince81 · 2017-04-12T07:43:18Z

controller/logoutcontroller.php

+            return new JSONResponse("No user found for $user", Http::STATUS_NOT_FOUND);
+        } else {
+            $this->userSession->setUser($user);
+            $this->logger->warning("Going to switch to a different user $oldUserId", ['app' => 'impersonate']);


Please use ->info here. Please check the specs, that's what was stated there.

PVince81 · 2017-04-12T07:44:34Z

appinfo/app.php

@@ -9,11 +9,19 @@
 * @copyright Jörn Friedrich Dreyer 2015
 */

+\OCP\App::registerAdmin('impersonate', 'settings-admin');


we might need two versions of the app: 9.1 using this approach and keep this line.
On 10.0 the panels can/must be registered with a different approach. Let's reconsider this later in a separate PR.

I'd make this OC10 compatible first. We are not going to backport these changes to 9.1 as they rely on core changes, eg https://github.com/owncloud/core/pull/27629/files#diff-7afe0377257ef7362517b1e27bda7ee6R66

@sharidas already found a way to make the app work without these core changes being necessary.

PVince81 · 2017-04-12T07:47:08Z

controller/settingscontroller.php

 		if ($user === null) {
 			return new JSONResponse("No user found for $userid", Http::STATUS_NOT_FOUND);
 		} else {
 			$this->logger->warning("changing to user $userid", ['app' => 'impersonate']);
 			$this->userSession->setUser($user);
+			\OC::$server->getAppConfig()->setValue('impersonate', 'impersonating_user_id', $oldUserId);


The app config is global for all users. This means that if two admins are impersonating different users, their values will be overwritten here. A better approach would be to use the user preferences.

But still, I don't think we should write anything to the database. The impersonating is only valid within this one PHP session, not across all PHP sessions of the same admin user.

Any reason why you decided to write this value to the DB ? I see you already write the old user to oldUserId in the session above.

PVince81 · 2017-04-12T07:47:29Z

js/impersonate.js

@@ -1,38 +1,47 @@
 (function(){

-	$(document).ready(function() {
+	$(window).load(function () {


Usually $(document).ready() is the better way.
Did that not work correctly ?

PVince81 · 2017-04-12T07:48:03Z

js/impersonate.js

 			$.post(
 				OC.generateUrl('apps/impersonate/user'),
 				{ userid: userid }
 			).done(function( result ) {
+				OC.AppConfig.setValue('impersonate','impersonating_user_id',currentUser);
 				window.location = OC.generateUrl('apps/files');


Use OC.redirect(url) instead which makes it possible to write JS unit tests for that.

PVince81 · 2017-04-12T07:48:44Z

js/impersonate.js

-			'</a></td>')
-			.insertAfter('#userlist .name');
+		OC.AppConfig.getValue('impersonate','impersonate_include_groups_list',"[]", function (data) {
+			data = eval(data);


Ouch, please avoid eval in JS at all costs.

If the data is JSON, please use the native JS JSON parser.

PVince81 · 2017-04-12T07:49:31Z

js/impersonate.js

+		OC.AppConfig.getValue('impersonate','impersonate_include_groups_list',"[]", function (data) {
+			data = eval(data);
+			if (data.length !== 0) {
+				var newColumn = $("#userlist").find("tr:first-child");


it is usually good practice to add a $ in the variable name when it's a jQuery object: $newColumn

PVince81 · 2017-04-12T07:50:18Z

js/impersonate.js

+				var newColumn = $("#userlist").find("tr:first-child");
+				$('<th id="impersonateId" scope="col">Impersonate</th>').insertAfter(newColumn.find("#headerName"));
+				for(var i = 0; i < data.length; i++) {
+					var collectTr = $("#userlist tr");


would this also select the header column ? I suggest scoping the selector to the "tbody"

PVince81 · 2017-04-12T07:50:50Z

js/impersonate.js

+						var textDisplayed = $(this).find('td.groups').text();
+						if ($.trim(textDisplayed) === $.trim(data[i])) {
+							var addImpersonate ='<td><a class="action permanent impersonate" href="#" title="' +
+								'Impersonate' + '">' +


string needs to be translated: t('impersonate', 'Impersonate')

PVince81 · 2017-04-12T07:51:40Z

js/impersonate_logout.js

+		).promise();
+
+		promisObj.done(function () {
+			window.location = OC.generateUrl('apps/files');


OC.redirect(url)

PVince81 · 2017-04-12T07:52:32Z

js/impersonate_logout.js

+		});
+	}
+
+	$("#logout").on('click', function () {


Does this block the default logout click ? If not there is a risk of race condition: if the page unloads faster then this code might not run at all. I suggest to add a e.preventDefault() in your handler or returning false.

PVince81 · 2017-04-12T07:54:24Z

js/impersonate.js

-			'<img class="svg permanent action" src="' + OC.imagePath('core','actions/user.svg') + '" />' +
-			'</a></td>')
-			.insertAfter('#userlist .name');
+		OC.AppConfig.getValue('impersonate','impersonate_include_groups_list',"[]", function (data) {


Why are you reading this setting with JS / ajax ? I saw in the PHP template that you already appended the current value into the DOM so no need to reload it again.

Or if you prefer the ajax way, then remove the PHP template one.

PVince81 · 2017-04-12T07:59:02Z

I haven't tested this yet but see another potential issue: the users list is usually limited to the first page of users (50 I think). Then every time you scroll down it will load 10 more users.

In this PR the code only seems to render the extra column to the already pre-rendered users. So when you scroll down, the newly appended rows will be missing the column.

To solve this, I suggest to use a different approach: whenever the page is loaded, hijack the method UserList.add and extend it to automatically add a new column. This way when the user list logic loads or adds new users, it will automatically run the logic for the new column.

Hijacking can be done this way:

var oldAdd = UserList.add;
UserList.add = function() {
    var $tr = oldAdd.apply(this, arguments);
    if (!$tr) {
        return;
    }
    // TODO: add here the code to inject a new column inside the $tr
    return $tr
};

You need to make sure that this hijacking/overriding of the method runs early enough before the users page ajax load, but late enough after users.js is loaded.

sharidas · 2017-04-12T17:29:56Z

@PVince81 @butonic I have made the changes in the new PR for review:

Removed the $_SESSION variable from the code and tried to use get and set methods from getSession.
Removed the reference of impersonate from the core. And tried to use addScript in the app.php
Used UserList.add. Replaced $(window).load with $(document).ready
Removed window.location and used OC.redirect as mentioned.
Removed the setValue of impersonating_user_id from the code
Replaced logger->warning to logger->info
String translation done.

PVince81 · 2017-04-13T07:20:29Z

controller/logoutcontroller.php

+		$user = $this->userManager->get($user);
+
+        if($user === null) {
+            return new JSONResponse("No user found for $user", Http::STATUS_NOT_FOUND);


The message is always "No user found for null", not very useful.

Oops it should have been userid.

PVince81 · 2017-04-13T07:21:50Z

controller/logoutcontroller.php

+            $newUserId = \OC::$server->getSession()->get('newUserId');
+            $oldUserId = \OC::$server->getSession()->get('oldUserId');
+            \OC::$server->getSession()->set('newUserId',$oldUserId);
+            \OC::$server->getSession()->set('oldUserId',$newUserId);


You should rather clear the old user id here. We don't want when the admin logs out again that they get switched back to the last impersonated user.

I have tried with remove() on oldUserId. And in my next version of PR I will update the section with the same.

PVince81 · 2017-04-13T07:22:30Z

controller/logoutcontroller.php

+            return new JSONResponse("No user found for $user", Http::STATUS_NOT_FOUND);
+        } else {
+            $this->userSession->setUser($user);
+            $this->logger->info("Going to switch to a different user $userid", ['app' => 'impersonate']);


"Switching back to previous user $userid" ?

"Switching back to previous user $userid" makes sense. Will be reflected in my next PR.

PVince81 · 2017-04-13T07:23:44Z

Just thought of a funny case, we need to make sure it doesn't cause problem:

login as admin1
impersonate admin2
now that you're admin2 you also have impersonate rights, impersonate user3
log out

Should log out go back to admin2 or admin1 ?
If possible I'd rather have log out go back to admin1.
But if currently it goes to admin2 it is fine and can be fixed in a separate PR.

PVince81 · 2017-04-13T07:25:58Z

js/impersonate.js

-			.insertAfter('#userlist .name');
+		var includedGroups;
+		OC.AppConfig.getValue('impersonate','impersonate_include_groups_list',"[]", function (data) {
+			includedGroups = $.parseJSON(data);


There is still a slight risk that the groups haven't finished loading if the users page loads faster, in which case the groups aren't available. Maybe might still need to find a way to pass these through the a PHP template.

I'm not sure how to do this because it is not possible to hook into the users page PHP template.

The sad thing is that the users list entries are rendered both by the PHP template and also by ajax. (in the future I want this to be ajax-only: owncloud/core#10994).

We'd need a way to hook into the user list and prevent it to load until the setting above is also loaded.
Or, a bit more complex, use a promise to load the included group list. Then when the list is used, use its then() callback to render the additional column.

Maybe this is too complex for now and can be done in a separate PR. (need to keep this in mind)

PVince81 · 2017-04-13T07:26:48Z

js/impersonate.js

+		$('<th id="impersonateId" scope="col">Impersonate</th>').insertAfter($newColumn.find("#headerName"));
+		UserList.add = function () {
+			var $tr = oldAdd.apply(this,arguments);
+			var groupsSelectedByUser = $tr.find('.groups').text();


I'm surprised that this is stored directly in the DOM and not in a data attribute, but ok

PVince81 · 2017-04-13T07:27:33Z

js/impersonate.js

+			var groupsSelectedByUser = $tr.find('.groups').text();
+			var found = false;
+			for(var i=0; i < includedGroups.length; i++) {
+				if($.trim(includedGroups[i]) === $.trim(groupsSelectedByUser)) {


hmm, does this work if the user is in multiple groups ?

PVince81 · 2017-04-13T07:28:17Z

js/impersonate.js

+				addImpersonate = '<td class="impersonateDisabled"><span></span></td>';
+				$(addImpersonate).insertAfter($tr.find('.name'));
+			}
+			if(!$tr) {


Do this at the beginning of the method, no need for all the complex above logic if there is no tr

PVince81 · 2017-04-13T07:29:25Z

js/impersonate_logout.js

@@ -0,0 +1,37 @@
+$(document).ready(function () {
+
+	$("#logout").attr("href","#");


We should only override the logout logic if the current user is being impersonated. (when oldUserId is defined)

Don't override logout when no impersonation is in progress.

Does logout still work in regular cases ?

PVince81 · 2017-04-13T07:30:09Z

appinfo/app.php

@@ -9,11 +9,14 @@
 * @copyright Jörn Friedrich Dreyer 2015
 */

+\OCP\App::registerAdmin('impersonate', 'settings-admin');
+\OCP\Util::addScript('impersonate','impersonate_logout');


I'd say only add this script if impersonating is already in progress.
Else don't override logout.

PVince81 · 2017-04-13T07:40:11Z

BUG: (optional) potential race condition between loading groups and the users list (maybe to fix separately)
BUG: (optional) inception-impersonating logout should return to initial user (see Improve impersonate app with changes #16 (comment))

I haven't tested this yet, just guessing from reading the code.
Please address my other comments and maybe we could merge first this if it works already.

sharidas · 2017-04-13T09:49:09Z

Reply to : #16 (comment)
Yes in my latest PR ( yet to upload), it would logout back to admin2.

PVince81 · 2017-04-13T10:18:22Z

TODO: if no groups were selected, show impersonate icon for every user
TODO ? don't show impersonate icon for currently logged user

PVince81 · 2017-04-13T10:28:09Z

js/impersonate.js

+			var found = false;
+			for(var i=0; i < includedGroups.length; i++) {
+				if($.inArray($.trim(includedGroups[i]),groupsSelectedByUser) !== -1) {
+					found = true;


Please break when found, this logic would add the button multiple times if multiple groups match.

Since you're touching this logic anyway, please also make sure that if no groups were selected on the settings page, then display the column for all users.

PVince81 · 2017-04-13T10:29:20Z

Did a quick test, works.

Please fix my last comment https://github.com/owncloud/impersonate/pull/16/files#r111357943 then I think we can merge this.

Below are the changes implemented: 1) Support of groups which can be impersonated. 2) Support of logout which switches back to orignal user. 3) Support to display the user logged as information as notification. 4) Removed the confirmation dialog to impersonate. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2017-04-13T11:47:23Z

I only tested a admin.

The requirement was to make this especially work for subadmins, the console shows an error in this scenario:

Create users "user1", "user2", "user3"
Set "user1" to "group1", "group2" and "group3"
Set "user2" to "group1"
Set "user3" to "group2"
Set "user1" as group admin of group "group1"
Only allow impersonating for "group1"
Login as "user1"
Go to users page

Expected result:

Can only impersonate for "user1", no errors

Actual:

PVince81 · 2017-04-13T12:51:42Z

Ok, so the bug in question seems to be due to the fact that the ajax endpoint for appconfig is only usable for admins, not subadmins. So the value can't be read that way.

We need to find a different solution to expose the list of groups, which would also solve https://github.com/owncloud/impersonate/pull/16/files#r111325730

PVince81 · 2017-04-13T12:51:49Z

Merging for now.

sharidas force-pushed the impersantionV2 branch from 67551bf to d21e355 Compare April 12, 2017 05:59

PVince81 reviewed Apr 12, 2017

View reviewed changes

butonic changed the title ~~Improvise impersonate app with changes~~ Improve impersonate app with changes Apr 12, 2017

sharidas force-pushed the impersantionV2 branch from d21e355 to 67f6be1 Compare April 12, 2017 17:12

PVince81 reviewed Apr 13, 2017

View reviewed changes

sharidas force-pushed the impersantionV2 branch from 67f6be1 to 8e29b70 Compare April 13, 2017 10:20

PVince81 reviewed Apr 13, 2017

View reviewed changes

sharidas force-pushed the impersantionV2 branch from 8e29b70 to 8e5f3bc Compare April 13, 2017 10:38

sharidas force-pushed the impersantionV2 branch from 8e5f3bc to acff904 Compare April 13, 2017 11:47

PVince81 merged commit 7be11ac into master Apr 13, 2017

PVince81 deleted the impersantionV2 branch April 13, 2017 12:52

PVince81 mentioned this pull request Apr 13, 2017

Subadmins do not work #18

Closed

		@@ -0,0 +1,37 @@
		$(document).ready(function () {

		$("#logout").attr("href","#");

Improve impersonate app with changes #16

Improve impersonate app with changes #16

Conversation

sharidas commented Apr 12, 2017

CLAassistant commented Apr 12, 2017 • edited Loading

sharidas commented Apr 12, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 12, 2017

sharidas commented Apr 12, 2017

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

Choose a reason for hiding this comment

PVince81 commented Apr 13, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 13, 2017

sharidas commented Apr 13, 2017

PVince81 commented Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Apr 13, 2017

PVince81 commented Apr 13, 2017 • edited Loading

PVince81 commented Apr 13, 2017

PVince81 commented Apr 13, 2017

CLAassistant commented Apr 12, 2017 •

edited

Loading

PVince81 commented Apr 13, 2017 •

edited

Loading

PVince81 commented Apr 13, 2017 •

edited

Loading