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

Fix permission checks on contact create popups #13506

Merged
merged 1 commit into from
Jan 31, 2019

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 25, 2019

Overview

The permissions checked to show buttons to create a new contact via entityRef were not the correct permissions to actually create a contact in a profile form, which meant that some users could see the buttons but not be able to add contacts.

Before

Removing "profile listings and forms" and "profile create" permissions from a user would result in the buttons still appearing but an error when clicking on it.

screenshot from 2019-01-25 16-32-38

After

Button appears correctly when user has permission. Note in screenshot the user lacks admin or profile permissions, so no create buttons are shown in the entityRef widget.

screenshot from 2019-01-25 16-33-21

@civibot
Copy link

civibot bot commented Jan 25, 2019

(Standard links)

@civibot civibot bot added the master label Jan 25, 2019
@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@colemanw I can replicate this & this PR fixes so that is good.

However, there is a meaningful change to how 'administer CiviCRM' interacts - prior to this PR it doesn't confer the permission to add profiles - this is somewhat meaningful as without having full access specific profiles can be permitted via ACLs (obviously in this place the code doesn't pick up this nuance but I feel like that could/ in an ideal world would be fixed).

In short - if giving additional rights to 'Administer CiviCRM' were a side thought I think we should remove it. If there is a case for it then we need to flush that out - possibly as a separate PR.

@colemanw
Copy link
Member Author

Ok fair enough. Yes it was a side-thought, as in theory someone with "administer CiviCRM" permission can do whatever they want. But I'm happy to table that discussion and simplify this PR.

@eileenmcnaughton eileenmcnaughton merged commit ca79616 into civicrm:master Jan 31, 2019
@eileenmcnaughton eileenmcnaughton deleted the profileCreate branch January 31, 2019 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants