Allow impersonating regular users + big refactoring of impersonations by deivid-rodriguez · Pull Request #3226 · decidim/decidim

deivid-rodriguez · 2018-04-15T19:45:58Z

🎩 What? Why?

This is a big branch with a lot of commits, sorry about that, but I've been doing very granular refactoring of the whole feature to make sure I don't break anything and that means a lot of tiny atomic commits. Let me know if I should break it up to make reviewing easier.

It's built on top of #3225 for convenience, but I can also detach it.

The feature

Every user can now be impersonated, not only managed users, but if it's a real non-managed user, the impersonating user needs to provide a reason which is logged for posterity. (I've assumed we don't want to allow impersonating admin users, we don't right?). As a result, the "Managed Users" section is now called "Impersonations", and it allows filtering users by name or their managed status in order to impersonate them (or view their impersonation logs).

The refactoring

This PR also adds a lot of refactorings of this part of the code, that I found it was hard to work with and now it's easier. This is a summary of the improvements:

The impersonation form is now "single step". Before there used to be an initial step to choose an authorization handler in case there were more than one. Now it's all managed together in a single form (if there's more than one authorization handler available, a select box is used which renders the correct form when the handler is changed). I find this is simpler in terms of usability and codewise too.
We no longer extend rectify forms in a weird ways like this one.
We no longer mutate rectify forms after they are created like here. This is something we hardly ever do and it means an eventual migration to a rectify alternative that depends on dry-struct and dry-types instead of virtus (which is discontinued) will be easier, since dry-struct's are read-only.
We no longer call commands from within commands like here. This strikes to me as a bit weird and something we hardly ever do, and it's not necessary after the refactorings.
The whole feature is DRY'ed up. Before all forms/controllers/commands were quasi-duplicated and they contained mostly the same code but different edge cases. Now the whole thing is unified, and the edge cases are easier to understand in my opinion.

📌 Related Issues

None.

📋 Subtasks

Add CHANGELOG entry

📷 Screenshots (optional)

mrcasals

Wow, nice refactor! Looks good to me!

mrcasals · 2018-04-16T07:33:26Z

Proposals specs are failing, I guess it's a flaky spec that fails randomly, I've rescheduled the job https://circleci.com/gh/decidim/decidim/84563

josepjaume · 2018-04-17T09:08:48Z

@deivid-rodriguez I assume this isn't WIP anymore?

deivid-rodriguez · 2018-04-17T10:04:56Z

It's a very big branch so I wanted to do a final self-review pass today, that's why I didn't switch the label yesterday. But it's pretty much ready!

deivid-rodriguez · 2018-04-17T10:58:36Z

If you want it to land it now, I can also do a followup PR later if I find anything.

deivid-rodriguez · 2018-04-17T11:05:54Z

Sorry, wrong button 😳

deivid-rodriguez · 2018-04-17T16:23:16Z

So I did a quick pass and I have a couple of doubts:

I moved the uniqueness validation down to the base authorization handler in c77d435. I partially ended up reverting that commit in b1c0479 because it was changing behavior , but the uniqueness validation stayed in the base handler. This looks more correct to me but not sure if some handlers are using the unique_id field but still relying on uniqueness not being enforced when calling valid? on the form. Thoughts? Not sure if I correctly explained the "fear".
I added a second dummy authorization handler in 7bb06ca. It was useful to try out the authorization handler selector but maybe it's overkill to have two dummy authorization handlers in the default generated application? I could remove it since tests don't really need it (they register an extra handler on the fly when necessary). Thoughts?

Other than these two nits (which might be ok, but I prefer to call for opinions), this looks good to me!

deivid-rodriguez · 2018-04-18T01:18:42Z

My own answers to my questions are that the first concern shouldn't be a problem but regarding the second concern I would remove the extra dummy handler I added to not complicate the dummy app any more.

Prefer fully qualified constants to `require_dependency`.

The functionality is separate from the impersonations stuff, and that controller is already complicated enough. By splitting it, we can also choose a more adequate name for the resource.

I forgot to add this when I worked on it at the very beginning of this branch.

Let it receive and arbitrary scope and not limit it to organization users.

Not sure about the specs for this, but I'll stay on the safe side.

For example the default action on the dummy component is called "Foo" so capybara actions are likely to conflict if we use that name.

josepjaume · 2018-04-18T13:07:35Z

@deivid-rodriguez it took me some time but I understood your doubts haha. As you said, I believe the first one shouldn't be a problem (famous last words) and I would, as you suggest, remove one of the two authorization handlers from the generated app.

Thanks for your work!

deivid-rodriguez · 2018-04-18T13:11:06Z

(famous last words)

xD Indeed.

Turns out removing the extra authorization broke the tests. That was unintented, fixing now.

This reverts commit fb327d6. I don't think we need this in the default application. Specs were actually relying on the existance of the global extra authorization handler, so they needed a bit of tweaking.

oriolgual

Nice work!

oriolgual · 2018-04-18T14:39:59Z

decidim-admin/app/commands/decidim/admin/impersonate_user.rb

        Decidim::ImpersonationLog.create!(
          admin: form.current_user,
          user: user,
+          reason: form.reason,


When impersonating an existing user I think we should send an email to them to let them when and why an admin has impersonated them. Otherwise without an audit no one would know about it.

It would be great to also notify the user when the impersonation has finished.

Uff, that seems reasonable but this is a new spec for me and I have no more availability to work on this... 😞

Note though that the idea of impersonations (at least as I see them), is that the impersonated user is present during the impersonation. I mean, that's why we ask for authorization every time, right?

We can do it in another PR @deivid-rodriguez, don't worry.

And yes, the user should be present during the impersonation, but and admin could know (or make a note) the user's data and impersonate them later.

We can do it in another PR @deivid-rodriguez, don't worry.

Thanks, and sorry for overlooking that!

And yes, the user should be present during the impersonation, but and admin could know (or make a note) the user's data and impersonate them later.

Ok, just wanted to make sure we're on the same page! 👍

mrcasals · 2018-04-19T07:10:19Z

Merging!

ghost assigned deivid-rodriguez Apr 15, 2018

ghost added the status: WIP label Apr 15, 2018

deivid-rodriguez force-pushed the feature/impersonations_everywhere branch 2 times, most recently from eaf94c1 to 91726ec Compare April 15, 2018 23:53

mrcasals previously approved these changes Apr 16, 2018

View reviewed changes

deivid-rodriguez dismissed mrcasals’s stale review via 6530aec April 16, 2018 16:44

deivid-rodriguez force-pushed the feature/impersonations_everywhere branch 3 times, most recently from e2c7bb1 to 2352cef Compare April 16, 2018 22:44

ghost assigned josepjaume Apr 17, 2018

deivid-rodriguez closed this Apr 17, 2018

ghost removed the status: WIP label Apr 17, 2018

deivid-rodriguez reopened this Apr 17, 2018

ghost added the status: WIP label Apr 17, 2018

deivid-rodriguez added 9 commits April 18, 2018 08:11

Normalize some controllers

cb61dac

Prefer fully qualified constants to `require_dependency`.

Rename query to be more generic

267605b

Use association in the model

6a6a0e2

Fix typos

c117c34

Tiny spec rewording

375be67

Allow reusing users query for other filterings

630643a

Fix typo

d2be9a4

Fix bad HTML

bb4f6f0

Spec wording fix

d0e0277

deivid-rodriguez added 11 commits April 18, 2018 08:11

Move impersonation list to separate controller

c9da44d

The functionality is separate from the impersonations stuff, and that controller is already complicated enough. By splitting it, we can also choose a more adequate name for the resource.

Indentation & whitespace

548a8eb

Fill in missing bits of functionality

b74bb1f

I forgot to add this when I worked on it at the very beginning of this branch.

Improve UI labelling to clarify functionality

7efac82

Generalize user filter query

1a1acd8

Let it receive and arbitrary scope and not limit it to organization users.

Disallow impersonating admins

34d7a66

Not sure about the specs for this, but I'll stay on the safe side.

Fix broken unit test

b4175a1

Another pass of locale cleaning

8c158f1

Rename test objects to reduce conflict chances

870abd6

For example the default action on the dummy component is called "Foo" so capybara actions are likely to conflict if we use that name.

Fix last minute regression that I found

410f38e

Add change log entries

02b28ce

deivid-rodriguez force-pushed the feature/impersonations_everywhere branch from 6cf2883 to c414e29 Compare April 18, 2018 11:14

Revert "Other dummy"

2f60233

This reverts commit fb327d6. I don't think we need this in the default application. Specs were actually relying on the existance of the global extra authorization handler, so they needed a bit of tweaking.

deivid-rodriguez force-pushed the feature/impersonations_everywhere branch from c414e29 to 2f60233 Compare April 18, 2018 13:48

oriolgual suggested changes Apr 18, 2018

View reviewed changes

oriolgual previously approved these changes Apr 18, 2018

View reviewed changes

deivid-rodriguez added in-review and removed status: WIP labels Apr 18, 2018

Merge branch 'master' into feature/impersonations_everywhere

853a13c

mrcasals dismissed oriolgual’s stale review via 853a13c April 19, 2018 07:09

ghost assigned mrcasals Apr 19, 2018

ghost added the status: WIP label Apr 19, 2018

mrcasals merged commit 103b9f3 into master Apr 19, 2018

mrcasals deleted the feature/impersonations_everywhere branch April 19, 2018 07:10

ghost removed the status: WIP label Apr 19, 2018

deivid-rodriguez mentioned this pull request Apr 26, 2018

Component permissions manifest #3300

Merged

4 tasks

mrcasals mentioned this pull request May 2, 2018

Fix controller name in admin routes #3323

Merged

Uh oh!

Conversation

deivid-rodriguez commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

The feature

The refactoring

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

mrcasals left a comment

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Apr 16, 2018

Uh oh!

josepjaume commented Apr 17, 2018

Uh oh!

deivid-rodriguez commented Apr 17, 2018

Uh oh!

deivid-rodriguez commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Apr 17, 2018

Uh oh!

deivid-rodriguez commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Apr 18, 2018

Uh oh!

josepjaume commented Apr 18, 2018

Uh oh!

deivid-rodriguez commented Apr 18, 2018

Uh oh!

oriolgual left a comment

Choose a reason for hiding this comment

Uh oh!

oriolgual Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

oriolgual Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oriolgual Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Apr 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deivid-rodriguez commented Apr 15, 2018 •

edited

Loading

deivid-rodriguez commented Apr 17, 2018 •

edited

Loading

deivid-rodriguez commented Apr 17, 2018 •

edited

Loading

deivid-rodriguez Apr 18, 2018 •

edited

Loading