Skip to content
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

#9179: Include editing support to allowed user groups #9210

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

dsuren1
Copy link
Contributor

@dsuren1 dsuren1 commented Jun 5, 2023

Description

This PR adds support to configure editingAllowedGroups to FeatureEditor and StyleEditor plugin

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Feature

Issue

What is the current behavior?

What is the new behavior?
The edit permission configuration follows the below hierarchy (Applicable for FeatureEditor and StyleEditor plugin)

  1. Allow all user to edit, when editingAllowedRoles has "ALL" configured in it
  2. Allow edit, when user role and group belongs to either editingAllowedRoles or editingAllowedGroups configured for the plugin

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@dsuren1 dsuren1 added this to the 2023.02.00 milestone Jun 5, 2023
@dsuren1 dsuren1 requested a review from offtherailz June 5, 2023 07:15
@dsuren1 dsuren1 self-assigned this Jun 5, 2023
@dsuren1 dsuren1 linked an issue Jun 5, 2023 that may be closed by this pull request
6 tasks
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dsuren1 . I tryed to suggest all the changes inline (initially only some review of code base to clarify it more) but I noticed during testing that one of the things didn't worked as expected.

In particular there is no way of allowing all logged users to be able to edit at the same time, without indicating all the groups of the system.This because the logic of the editing allowed selector for groups requires that the role is in the list.

I suggested to change this logic in the main selectorCreator used but you may need to test and check it more (and fix the unit tests).

We should handle and prioritize permissions this way:

  • "editingAllowedRoles": ["ADMIN"], editingAllowedGroups: ["geosolutions"] means all admins and members of geosolutions group.
  • "editingAllowedRoles": ["ADMIN", "USER"] means - all logged users (admins or users) so groups in this case are not important.
    So:
  • Please check if the logic i suggested works and the documentation fits this requirement, fixing unit tests
  • I wasn't able to test the styler. Please make you sure that this logic is correctly applied to the styler too.

web/client/selectors/security.js Outdated Show resolved Hide resolved
web/client/selectors/styleeditor.js Outdated Show resolved Hide resolved
web/client/selectors/styleeditor.js Outdated Show resolved Hide resolved
web/client/selectors/__tests__/security-test.js Outdated Show resolved Hide resolved
web/client/selectors/__tests__/security-test.js Outdated Show resolved Hide resolved
web/client/selectors/featuregrid.js Outdated Show resolved Hide resolved
web/client/selectors/featuregrid.js Outdated Show resolved Hide resolved
web/client/selectors/featuregrid.js Outdated Show resolved Hide resolved
web/client/plugins/StyleEditor.jsx Outdated Show resolved Hide resolved
web/client/plugins/FeatureEditor.jsx Outdated Show resolved Hide resolved
@dsuren1
Copy link
Contributor Author

dsuren1 commented Jun 6, 2023

Hi @offtherailz
Thanks for the feedback.

In particular there is no way of allowing all logged users to be able to edit at the same time, without indicating all the groups of the system.This because the logic of the editing allowed selector for groups requires that the role is in the list.

I suggested to change this logic in the main selectorCreator used but you may need to test and check it more (and fix the unit tests).

We should handle and prioritize permissions this way:

  • "editingAllowedRoles": ["ADMIN"], editingAllowedGroups: ["geosolutions"] means all admins and members of geosolutions group.
  • "editingAllowedRoles": ["ADMIN", "USER"] means - all logged users (admins or users) so groups in this case are not

This deviates from the requirement, the requirement was to allow ADMIN regardless of group the user belongs, and consider the group only when user is NON-ADMIN. For NON-ADMIN, default allowedGroups is everyone. With the current implementation, this configuration editingAllowedRoles: ["ADMIN", "USER"] will work for all the users of Mapstore. Kindly correct me if I'm missing something.

@dsuren1 dsuren1 requested a review from offtherailz June 6, 2023 11:41
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I can also see the requests to the rest interface for the configured group, even if he is not allowed. We may allow some special group for styler (e.g. adding a group styler and configuring members of this group) to make this feature fully testable.

@tdipisa tdipisa merged commit d839823 into geosolutions-it:master Jun 7, 2023
@tdipisa
Copy link
Member

tdipisa commented Jun 7, 2023

@ElenaGallo please test this in DEV taking into account the comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include editing support to allowed user groups
3 participants