Action authorizations at resource levels by leio10 · Pull Request #3804 · decidim/decidim

leio10 · 2018-07-09T10:42:59Z

🎩 What? Why?

This PR adds the ability to configure authorization actions at a resource level. It reuses all the permissions workflow developed for components, but allowing setting different configurations for different resources in the same component.

As with components, possible actions could be defined for resources. If the same action is defined both for a component and for a resource inside the component, it can be set at both levels. The authorization process will look for settings at a resource level first, and at a component level if not found.

I have two main reasons to introduce this change. The first reason is to give admin users the ability to define a set of related resources (proposals, meetings or any other resource) without having to use the same authorization settings for all of them.

The other reason is to allow authorization handlers to receive a reference to the resource that is affected by the action that the user is performing. This allows it to determine if a user can perform each action or not based on the resource information, in addition to the action itself and the user information. For example, this can be used to restrict a user to vote proposals by postal code or scope.

This are the steps for developers to use this feature:

Define actions for resources in the component manifest, as its done with components.
Add a links the permissions page for each resource in the admin zone, using the edit_component_permissions_path helper method.
Pass an extra resource named parameter to the calls to the authorized? method and to to the action_authorized_link_to and action_authorized_button_to helpers.

In this PR I also implemented this feature for budgets (vote action), meetings (join) and proposals (vote and endorse). I think that this could be useful, but if it's not or it adds too much complexity to the UI, I can revert those changes. Also, I can add some code to allow to enable/disable this feature globally in the decidim initializer, or to do it by component in the admin panel. Or I can do both. 😄

📌 Related Issues

Based on AJAX authorization modals #3753, as it needs dynamic modals to work.

📋 Subtasks

Add tests
Add documentation regarding the feature
Add CHANGELOG entry

📷 Screenshots (optional)

leio10 · 2018-07-09T11:05:04Z

Hi @mrcasals, here am I again with extra work for you. I promise this is the last PR of this kind for a while. 😬

@decidim/developers I have a technical concern that I want to comment: In the admin zone, I'm reusing the existing command (UpdateComponentPermission) and controller (ComponentPermissionsController) to deal with resources permissions, since the logic is very similar. I didn't renamed it to avoid breaking compatibility and increase even more the PR size, but I can do it if you consider that is needed.

leio10 · 2018-07-09T11:29:25Z

@decidim/product In this PR I made some choices that I'm very interested in having your opinion and approval:

When a resource has specific permissions settings, it's icon is highlighted as shown in the following image. I think that this could be useful to increase the awareness of this setting to the admin users. But this doesn't happen in the components listing or with any other icon, so I can remove it if you think that it is not consistent with the rest of the application or it overemphasizes this feature respect the others.
I restricted the ability to set resources permissions only to users that can update permissions for that component. I don't know if this makes any difference with the current user roles we have, but I think that is the best option for future roles improvements.

mrcasals · 2018-07-09T12:33:50Z

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

@@ -1,26 +1,28 @@
 # frozen_string_literal: true

+require "hashdiff"


We should move this to the decidim-admin/lib/decidim/admin/engine.rb file, I think

mrcasals · 2018-07-09T12:34:57Z

decidim-admin/app/controllers/decidim/admin/component_permissions_controller.rb

+      def resource
+        @resource ||= if params[:resource_id] && params[:resource_name]
+                        manifest = Decidim.find_resource_manifest(params[:resource_name])
+                        manifest&.resource_scope(component)&.find_by(id: params[:resource_id])


Coverage reports these lines as not tested, is that right?

Yes, I'm completing participatory spaces system tests to cover this case 👍

@leio10 nice, tell me when it's ready for a final review! 😄

mrcasals · 2018-07-09T12:38:44Z

Wow, really nice work @leio10! Thanks for your contributions!

I left a couple of comments there, but code looks awesome 👏

leio10 · 2018-07-10T10:24:42Z

@mrcasals Tests added! Do you think we need seeds for this feature? I didn't find seeds for regular component permissions, so I guess that the answer is no.

mrcasals · 2018-07-10T10:26:18Z

@leio10 no, I don't think that's necessary!

mrcasals · 2018-07-10T10:27:46Z

@leio10 should we wait for an answer from @decidim/product before merging this?

leio10 · 2018-07-10T10:40:32Z

@mrcasals I don't know 😁. I guess that we can wait if there is anyone available on these days who can review this feature. Also, we can merge this as is, and I can add additional PRs in the future if any change is needed.
Meanwhile, I'm going to add a CHANGELOG entry and complete the docs to get this PR ready to be merged.

andreslucena · 2018-07-10T11:18:55Z

should we wait for an answer from @decidim/product before merging this?

For our part we like this feature on a first sight, although I can't see that clear what kind of processes could need these kind of granularity. Maybe @leio10 can give us more details (like an User Story)?

isaacmg410 · 2018-07-10T12:43:59Z

@leio10 Good Job!! 👏

I think it would be interesting to extend this functionality for 'participatory_space' as well. Especially to be able to use these helpers 'action_authorized_link_to' and 'action_authorized_button_to' in a 'participatory_space' What do you think?

cc// @mrcasals

deivid-rodriguez · 2018-07-10T12:55:58Z

Kudos from me too, @leio10, awesome job!

I have two main reasons to introduce this change. The first reason is to give admin users the ability to define a set of related resources (proposals, meetings or any other resource) without having to use the same authorization settings for all of them.

The other reason is to allow authorization handlers to receive a reference to the resource that is affected by the action that the user is performing. This allows it to determine if a user can perform each action or not based on the resource information, in addition to the action itself and the user information. For example, this can be used to restrict a user to vote proposals by postal code or scope.

Both situations seem like very nice features to have! 👍

I think it would be interesting to extend this functionality for 'participatory_space' as well. Especially to be able to use these helpers 'action_authorized_link_to' and 'action_authorized_button_to' in a 'participatory_space' What do you think?

What special actions do we have at the participatory space level, I can't seem to recall any... In any case, I think adding something this should be discussed separately from this PR.

mrcasals · 2018-07-10T13:23:28Z

I think it would be interesting to extend this functionality for 'participatory_space' as well. Especially to be able to use these helpers 'action_authorized_link_to' and 'action_authorized_button_to' in a 'participatory_space' What do you think?

I'm with @deivid-rodriguez, let's discuss this somewhere else @isaacmg410 😄

leio10 · 2018-07-10T13:48:27Z

Thanks for the feedback @andreslucena!

Before giving some examples, I will try to explain what I think is the key to answer your question: the way of using authorizations handlers and all the naming discussion around it. I understand that they where created to allow to authenticate users against an official census. Once an authorization is granted, users won't be authorized anymore against it. I understand that in this scenario this feature doesn't add too much advantages.

On the other hand, with PR #2438 we also allow to app developers to override this behavior, allowing to use authorization handlers to authorize users on each action performed (in other words, to ask: are you still in the census?). They also improve the ability for admin users to configure requirements to perform actions through options (are you older that 18? where do you participate? which document did you use to register in the census? which is your level of membership?). I think that in this scenario the need to have different authorization settings for different resources is clearer. Here are some examples that I can think (some are real, some aren't 😄):

Proposals related to a scope could only be voted by users that live in that scope.
Users could require specific membership level to join a meeting.
Legal restrictions could deny some users to vote for certain projects in a participatory budget.
Users could be required to be registered before to a date to vote in an electronic voting.

Some of these restrictions could be implemented creating different components or participatory spaces and configuring differently in each case, but this would force admin users to group resources by permission needs instead of what is logical to end users.

To summarize, I know that this can be useful or not based on the needs of each organization. That is why I was worried about adding extra complexity to users that will not use this feature. And that is why I suggested that I can remove it from any or all of the modified modules (budgets, meetings and proposals) or implement the option to disable it globally by code, by component in the admin zone or in both places.

leio10 · 2018-07-10T13:57:13Z

Thanks @isaacmg410!

As @deivid-rodriguez said, currently participatory spaces don't have related actions, so currently we can't apply this improvement there.

On the other hand, I don't know if I'm misunderstanding you, but I've seen the use of action_authorized_..._to helpers used just to check if the user is logged or not. So, if that is what you are referring to, I'm glad to comment that since #3753 we have logged_link_to and logged_button_to that can be used anywhere without having to deal with authorization handlers complexity.

arnaumonty · 2018-07-10T14:52:38Z

Hi @leio10 This is a great feature and contributes a lot to the grained permission strategy (related to #2056 in the user roles) that we have. Some comments below:

When a resource has specific permissions settings, it's icon is highlighted as shown in the following image. I think that this could be useful to increase the awareness of this setting to the admin users. But this doesn't happen in the components listing or with any other icon, so I can remove it if you think that it is not consistent with the rest of the application or it overemphasizes this feature respect the others.

It is a good idea to increase the awareness. It is a very important feature for admins and it looks good to me to highlight the icon.

I restricted the ability to set resources permissions only to users that can update permissions for that component. I don't know if this makes any difference with the current user roles we have, but I think that is the best option for future roles improvements.

+1

To summarize, I know that this can be useful or not based on the needs of each organization. That is why I was worried about adding extra complexity to users that will not use this feature. And that is why I suggested that I can remove it from any or all of the modified modules (budgets, meetings and proposals) or implement the option to disable it globally by code, by component in the admin zone or in both places.

Implement the option to disable/enable it by component in the admin zone could be a great solution.

From @decidim/product +1 to go ahead with this.

leio10 · 2018-07-10T16:03:36Z

Great @arnaumonty!!

I just implemented the enable/disable option and updated the docs and the changelog. I left the option enabled in all the modules, tell me if you prefer to be disabled by default in any or all of them.
Tomorrow I'll implement new tests for this if needed and I think that then we can merge this if there is no any other concern. 🎉

The code was already in CardMCell and two more classes, and I was needing it in `JoinMeetingButtonCell`, so it seems better to move it to the base class for all cells.

This allows to inform the user that the resource has different permissions settings from the component settings

leio10 · 2018-07-11T12:38:22Z

decidim-core/lib/decidim/resourceable.rb

+
+      # Public: Whether the permissions for this object actions can be set at resource level.
+      def allow_resource_permissions?
+        false


This is the only line without coverage, because this method is overriden by all resourceables models.

I can change it to

def allow_resource_permissions?; end

and it would work well, but I think is would be less clear.

I also can add a new dummy resource class that doesn't override this method, but I think that it doesn't worth it.

It's OK for me as it is! 😄

Does it need to be overridden? If so maybe add a raise NotImplementedError?

NotImplementedError is not intended for this use case, I think...

https://ruby-doc.org/core-2.2.0/NotImplementedError.html

@oriolgual it doesn't need to be overridden. By default, resources shouldn't allow to set permissions at that level.

Also, I just realized that there are a lot of other resourceable models, we can just add a test for any of that model and complete the coverage.

leio10 · 2018-07-11T12:38:41Z

@mrcasals I think this PR is ready to be reviewed.

mrcasals · 2018-07-11T14:06:06Z

@leio10 thank you very much for your contributions! Much appreciated! 😄

ghost assigned leio10 Jul 9, 2018

ghost added the status: WIP label Jul 9, 2018

leio10 force-pushed the enhancement/resource_action_authorization branch 2 times, most recently from 4c3d960 to f32c6b9 Compare July 9, 2018 10:55

mrcasals added in-review and removed status: WIP labels Jul 9, 2018

mrcasals reviewed Jul 9, 2018

View reviewed changes

ghost added the status: WIP label Jul 10, 2018

mrcasals removed the in-review label Jul 10, 2018

leio10 force-pushed the enhancement/resource_action_authorization branch 4 times, most recently from 8b11caa to e5bd64a Compare July 11, 2018 10:04

leio10 added 2 commits July 11, 2018 12:55

Add url helpers to all cells

0081fa8

The code was already in CardMCell and two more classes, and I was needing it in `JoinMeetingButtonCell`, so it seems better to move it to the base class for all cells.

Add permissions to resources

e690120

leio10 added 18 commits July 11, 2018 12:55

Allow to pass a resource to the authorization workflow

46bc5de

Allow to define actions for resources in a component definition

0cf5520

Allow to set permissions for a specific resource in admin zone

75a5f01

Add class to highlight permissions icon in resources lists

7cc10b3

This allows to inform the user that the resource has different permissions settings from the component settings

Implement resources permissions for budgets

d286749

Implement resources permissions for meetings

1759a02

Implement resources permissions for proposals

bf284a0

Rename dummy class name to reduce names missunderstandings

2a8ea0a

Allow to unset permissions at resource level when set at component level

254b811

Missing tests added

b85397b

Allow to disable permissions at resource level

3be2f0d

Moved "link to resource permissions" to a helper and allow to disable it

6385779

Allow to disable resource permissions in budgets

10d9766

Allow to disable resource permissions in meetings

68585cf

Allow to disable resource permissions in proposals

f83dee7

Cache authorization result for component when has no resource permission

d7e1831

Authorizable actions docs updated

4427fb2

Add CHANGELOG entry

755137a

leio10 force-pushed the enhancement/resource_action_authorization branch from e5bd64a to 755137a Compare July 11, 2018 11:50

leio10 commented Jul 11, 2018

View reviewed changes

leio10 removed the status: WIP label Jul 11, 2018

leio10 changed the title ~~[WIP] Action authorizations at resource levels~~ Action authorizations at resource levels Jul 11, 2018

leio10 added the in-review label Jul 11, 2018

oriolgual approved these changes Jul 11, 2018

View reviewed changes

mrcasals merged commit 28abc87 into decidim:master Jul 11, 2018

leio10 deleted the enhancement/resource_action_authorization branch July 11, 2018 14:24

amiedes mentioned this pull request Dec 18, 2018

Support requiring multiple authorizations for voting/endorsing items #4677

Closed

PierreMesure mentioned this pull request Oct 27, 2020

Fix a mistake in a string #6750

Merged

		@@ -1,26 +1,28 @@
		# frozen_string_literal: true

		require "hashdiff"

Uh oh!

Conversation

leio10 commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

leio10 commented Jul 9, 2018

Uh oh!

leio10 commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Jul 9, 2018

Uh oh!

leio10 commented Jul 10, 2018

Uh oh!

mrcasals commented Jul 10, 2018

Uh oh!

mrcasals commented Jul 10, 2018

Uh oh!

leio10 commented Jul 10, 2018

Uh oh!

andreslucena commented Jul 10, 2018

Uh oh!

isaacmg410 commented Jul 10, 2018

Uh oh!

deivid-rodriguez commented Jul 10, 2018

Uh oh!

mrcasals commented Jul 10, 2018

Uh oh!

leio10 commented Jul 10, 2018

Uh oh!

leio10 commented Jul 10, 2018

Uh oh!

arnaumonty commented Jul 10, 2018

Uh oh!

leio10 commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oriolgual Jul 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leio10 commented Jul 11, 2018

Uh oh!

mrcasals commented Jul 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

leio10 commented Jul 9, 2018 •

edited

Loading

leio10 commented Jul 9, 2018 •

edited

Loading

oriolgual Jul 11, 2018 •

edited

Loading