Add option for all group admins to impersonate their group memebers #134

sharidas · 2019-01-04T13:31:01Z

Add an option for all group admins to impersonate
their group members.

Signed-off-by: Sujith H [email protected]

codecov · 2019-01-04T13:39:09Z

Codecov Report

Merging #134 into master will increase coverage by 0.59%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #134      +/-   ##
============================================
+ Coverage     63.54%   64.14%   +0.59%     
- Complexity       29       31       +2     
============================================
  Files             7        7              
  Lines           192      198       +6     
============================================
+ Hits            122      127       +5     
- Misses           70       71       +1

Impacted Files	Coverage Δ	Complexity Δ
templates/settings-admin.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
controller/settingscontroller.php	`100% <100%> (+1.23%)`	`17 <1> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37445b6...3a3e44e. Read the comment docs.

codecov · 2019-01-04T13:39:10Z

Codecov Report

Merging #134 into master will decrease coverage by 0.91%.
The diff coverage is 76.47%.

@@             Coverage Diff              @@
##             master     #134      +/-   ##
============================================
- Coverage     63.54%   62.62%   -0.92%     
- Complexity       29       31       +2     
============================================
  Files             7        7              
  Lines           192      198       +6     
============================================
+ Hits            122      124       +2     
- Misses           70       74       +4

Impacted Files	Coverage Δ	Complexity Δ
templates/settings-admin.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
controller/settingscontroller.php	`96.47% <86.66%> (-2.3%)`	`17 <1> (+2)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d10bdd0...9349e5a. Read the comment docs.

sharidas · 2019-01-08T07:52:49Z

Current state of the PR:

Added radio buttons to select either grant impresonate access to all group admins or grant selected group admins.
Updated the js code accordingly.
Removed code duplication of impersonation and created a private method.
Updated the unit test accordingly.

Kindly review this change set @PVince81

sharidas · 2019-01-09T11:34:53Z

@tomneedham Require your helping hand in reviewing this PR.

tomneedham · 2019-01-31T13:07:39Z

Clean 10.0.10 install with this PR, and I get an issue with the user list:

Only one local user, who is admin. Am logged in as that user.

Ideas?

tomneedham

See above comments

sharidas · 2019-01-31T13:22:51Z

Clean 10.0.10 install with this PR, and I get an issue with the user list:

Only one local user, who is admin. Am logged in as that user.

Ideas?

On my local machine I executed the commands below for impersonate app:

 sujith@sujith-ownCloud  ~/test/owncloud3/apps/impersonate   allow-allgroupadmins  make clean
rm -fR build
rm -f js/templates/impersonateNotification.handlebars.js js/templates/addImpersonateIcon.handlebars.js js/templates/removeImpersonateIcon.handlebars.js
rm -fr /home/sujith/test/owncloud3/apps/impersonate/node_modules
 sujith@sujith-ownCloud  ~/test/owncloud3/apps/impersonate   allow-allgroupadmins  make dist
/usr/bin/npm install /home/sujith/test/owncloud3/apps/impersonate && touch node_modules
added 9 packages from 43 contributors and audited 10 packages in 1.559s
found 0 vulnerabilities



   ╭───────────────────────────────────────────────────────────────╮
   │                                                               │
   │      New minor version of npm available! 6.4.1 -> 6.5.0       │
   │   Changelog: https://github.com/npm/cli/releases/tag/v6.5.0   │
   │               Run npm install -g npm to update!               │
   │                                                               │
   ╰───────────────────────────────────────────────────────────────╯

/home/sujith/test/owncloud3/apps/impersonate/node_modules/handlebars/bin/handlebars -n "OCA.Impersonate" js/templates/impersonateNotification.handlebars -f js/templates/impersonateNotification.handlebars.js
/home/sujith/test/owncloud3/apps/impersonate/node_modules/handlebars/bin/handlebars -n "OCA.Impersonate" js/templates/addImpersonateIcon.handlebars -f js/templates/addImpersonateIcon.handlebars.js
/home/sujith/test/owncloud3/apps/impersonate/node_modules/handlebars/bin/handlebars -n "OCA.Impersonate" js/templates/removeImpersonateIcon.handlebars -f js/templates/removeImpersonateIcon.handlebars.js
mkdir -p build/impersonate
cp --parents -r \
        appinfo \
        controller \
        css \
        img \
        js \
        l10n \
        lib \
        README.md \
        LICENSE \
        CHANGELOG.md \
        templates \
        build/impersonate
rm -f build/impersonate/js/templates/*.handlebars
Skipping signing, either no key and certificate found in /home/sujith/.owncloud/certificates/impersonate.key and /home/sujith/.owncloud/certificates/impersonate.crt or occ can not be found at /home/sujith/test/owncloud3/apps/impersonate/../../occ
tar -czf build/impersonate.tar.gz -C /home/sujith/test/owncloud3/apps/impersonate/build/impersonate ../impersonate
tar: Removing leading `../' from member names
 sujith@sujith-ownCloud  ~/test/owncloud3/apps/impersonate   allow-allgroupadmins  cd -
~/test/owncloud3
 sujith@sujith-ownCloud  ~/test/owncloud3   fix-preview-image-orientation  ./occ a:e impersonate
Cannot load Xdebug - it was already loaded
impersonate enabled
 sujith@sujith-ownCloud  ~/test/owncloud3   fix-preview-image-orientation 

Navigated to the users page and created a new user user1 and the page looks like this

@tomneedham let me know if this is the same procedure followed? Because in my local instance it's not throwing issues.

tomneedham · 2019-01-31T15:22:30Z

So you're required to run make dist to build the app. Can you add this to the readme? Or change makefile to build on make like other apps do normally. And package on make dist.

tomneedham · 2019-01-31T15:27:02Z

Fresh install:

enable app
add some users
add some groups
user list shows impersoantion icons for all users !self
click on icon "cannot impersonate"

This is bad UX imo. Either dont show the icon if its not yet configured at all, or make a default mode so this works out of the box. The message also doesnt tell you that its not configured - so will never work. We will just get bug reports about this straight away.

templates/settings-admin.php

Add an option for all group admins to impersonate their group members. Signed-off-by: Sujith H <[email protected]>

sharidas · 2019-02-01T11:59:43Z

So you're required to run make dist to build the app. Can you add this to the readme? Or change makefile to build on make like other apps do normally. And package on make dist.

Created #135. IMHO, this does not fit to the PR. So I will raise it separately.

sharidas · 2019-02-01T12:03:55Z

This is bad UX imo. Either dont show the icon if its not yet configured at all, or make a default mode so this works out of the box. The message also doesnt tell you that its not configured - so will never work. We will just get bug reports about this straight away.

Agreed I have created a ticket #136. Again would be nice to create a separate PR imho.

sharidas · 2019-02-01T12:07:10Z

@tomneedham I have updated the changes requested at

Add option for all group admins to impersonate their group memebers #134 (comment)
Add option for all group admins to impersonate their group memebers #134 (comment)

Let me know if this looks ok.

tomneedham · 2019-02-01T17:05:30Z

Agreed I have created a ticket #136. Again would be nice to create a separate PR imho.

Ok. But #136 is more severe. The app is broken unusable at initial install as it is not configured. IMO a release blocker.

PVince81 · 2019-02-04T08:30:31Z

How was the UX before this option ? Was impersonate enabled by default for everyone ?

@sharidas I agree with @tomneedham that we need to fix the UX in the context of this PR

sharidas · 2019-02-04T11:20:55Z

The UX flow before this PR ( obviously enabled impersonate app in the oC instance ):

For `admin` user:

When admin user navigates to the users page , except admin all users will show up the impersonate icon
1. The failure from UI for impersonation happens when:
  1. A user tries to impersonate to the user, who has not logged in yet.
  2. Nested impersonation. That is user1 impersonates user2. And then as user2 no more impersonation to user3 or so.
  3. A normal user cannot impersonate admin user
  4. and so on ( there are more scenarios in https://github.com/owncloud/impersonate/blob/master/controller/settingscontroller.php )

For group admin user:

Groups when whitelisted in the settings page, all the users under the group show up the impersonate icon, except the group admin.
1. The failure from UI for impersonation happen when:
  1. A user tries to impersonate to the user, who has not logged in yet.
  2. Nested impersonation. That is user1 impersonates user2. And then as user2 no more impersonation to user3 or so.

Why do we have impersonation not possible if the user is not logged in once?

ref Impersonate a non logged in user causes exception #17
I totally agree that its not a good UI or say UX doesn't help user understand looking at the message from the dialog box message ( due to security reasons we cannot show up the exact message ).

I believe I have shared most of the details why the UX behaves the way it is. Let me know if we can take the issues pointed out here to a separate issue ( #135, #136 )

If there is still missing information, please let me know. I can help here.

sharidas · 2019-02-06T07:13:35Z

@tomneedham Let me know your suggestion/opinion, please.

tomneedham · 2019-02-12T13:01:40Z

Nested impersonation

We can detect this, and hide the icon. New issue.

tomneedham · 2019-02-12T13:02:19Z

3\. A normal user cannot impersonate admin user

Not relevant, since only loaded for admins.

tomneedham · 2019-02-12T13:03:02Z

1. A user tries to impersonate to the user, who has not logged in yet.

Javascript can check last login column, if never, dont show impersoantion. Or show tooltip with greyed out icon.

tomneedham · 2019-02-12T13:03:39Z

Anyway, conclusion here is this is no worse that current.

sharidas · 2019-02-12T13:25:58Z

Anyway, conclusion here is this is no worse that current.

Agreed, IMHO we can take the hiding of icons to a separate ticket. Let me know if this sounds reasonable.

sharidas added the 2 - Developing label Jan 4, 2019

sharidas added this to the development milestone Jan 4, 2019

sharidas self-assigned this Jan 4, 2019

sharidas requested a review from PVince81 January 4, 2019 13:31

sharidas mentioned this pull request Jan 4, 2019

[FR] Option to always allow subadmins to impersonate their group members #132

Closed

sharidas force-pushed the allow-allgroupadmins branch 2 times, most recently from 9be6a99 to 263ae55 Compare January 6, 2019 13:29

sharidas changed the title ~~[WIP] Add option for all group admins to impersonate their group memebers~~ Add option for all group admins to impersonate their group memebers Jan 8, 2019

sharidas added 3 - To Review and removed 2 - Developing labels Jan 8, 2019

PVince81 requested a review from tomneedham January 8, 2019 15:11

tomneedham suggested changes Jan 31, 2019

View reviewed changes

sharidas force-pushed the allow-allgroupadmins branch from 263ae55 to fa375ca Compare January 31, 2019 13:23

tomneedham reviewed Jan 31, 2019

View reviewed changes

templates/settings-admin.php Outdated Show resolved Hide resolved

tomneedham reviewed Jan 31, 2019

View reviewed changes

templates/settings-admin.php Outdated Show resolved Hide resolved

Add option for all group admins to impersonate their group memebers

04dd905

Add an option for all group admins to impersonate their group members. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the allow-allgroupadmins branch from fa375ca to 04dd905 Compare February 1, 2019 12:05

tomneedham mentioned this pull request Feb 1, 2019

On initial install impersonations icons shown but not possible because not configured #136

Open

PVince81 assigned tomneedham Feb 8, 2019

Merge branch 'master' into allow-allgroupadmins

3a3e44e

tomneedham approved these changes Feb 12, 2019

View reviewed changes

sharidas mentioned this pull request Feb 12, 2019

Hide icons where impersonation not possible in UI #138

Open

PVince81 mentioned this pull request Feb 12, 2019

Impersonate 0.5.0 #139

Closed

26 tasks

PVince81 merged commit 7341a80 into master Feb 12, 2019

delete-merged-branch bot deleted the allow-allgroupadmins branch February 12, 2019 15:58

PVince81 modified the milestones: development, QA Feb 19, 2019

sharidas mentioned this pull request Feb 19, 2019

Add option to always allow group admins to impersonate their group members owncloud/docs#622

Closed

12 tasks

This was referenced Mar 1, 2019

Tracking Test Results for Impersonate 0.5.0 #141

Closed

Group list from Allow group admins of specific groups to impersonate the users within those groups should be updated if we delete a group in the server #142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option for all group admins to impersonate their group memebers #134

Add option for all group admins to impersonate their group memebers #134

sharidas commented Jan 4, 2019

codecov bot commented Jan 4, 2019 •

edited

Loading

codecov bot commented Jan 4, 2019

sharidas commented Jan 8, 2019 •

edited

Loading

sharidas commented Jan 9, 2019

tomneedham commented Jan 31, 2019

tomneedham left a comment

sharidas commented Jan 31, 2019

tomneedham commented Jan 31, 2019

tomneedham commented Jan 31, 2019

sharidas commented Feb 1, 2019

sharidas commented Feb 1, 2019 •

edited

Loading

sharidas commented Feb 1, 2019

tomneedham commented Feb 1, 2019

PVince81 commented Feb 4, 2019

sharidas commented Feb 4, 2019 •

edited

Loading

sharidas commented Feb 6, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

sharidas commented Feb 12, 2019

Add option for all group admins to impersonate their group memebers #134

Add option for all group admins to impersonate their group memebers #134

Conversation

sharidas commented Jan 4, 2019

codecov bot commented Jan 4, 2019 • edited Loading

Codecov Report

codecov bot commented Jan 4, 2019

Codecov Report

sharidas commented Jan 8, 2019 • edited Loading

sharidas commented Jan 9, 2019

tomneedham commented Jan 31, 2019

tomneedham left a comment

Choose a reason for hiding this comment

sharidas commented Jan 31, 2019

tomneedham commented Jan 31, 2019

tomneedham commented Jan 31, 2019

sharidas commented Feb 1, 2019

sharidas commented Feb 1, 2019 • edited Loading

sharidas commented Feb 1, 2019

tomneedham commented Feb 1, 2019

PVince81 commented Feb 4, 2019

sharidas commented Feb 4, 2019 • edited Loading

For admin user:

For group admin user:

Why do we have impersonation not possible if the user is not logged in once?

sharidas commented Feb 6, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

tomneedham commented Feb 12, 2019

sharidas commented Feb 12, 2019

codecov bot commented Jan 4, 2019 •

edited

Loading

sharidas commented Jan 8, 2019 •

edited

Loading

sharidas commented Feb 1, 2019 •

edited

Loading

sharidas commented Feb 4, 2019 •

edited

Loading

For `admin` user: