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.

Sign up for GitHub

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

Jump to bottom

dev/core#1327 Implement new view status checks permission (reduce noize) #14521

Closed
wants to merge 2 commits into from
Closed

dev/core#1327 Implement new view status checks permission (reduce noize) #14521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ae0807
Implement new view status checks permission as per NY Sprint
seamuslee001 Jun 13, 2019
a1eddd0
Add in transitional arrangement so that administer CiviCRM is still s…
seamuslee001 Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion CRM/Core/Invoke.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,17 @@ public static function form($action, $contact_type, $contact_sub_type) {
* @param CRM_Core_Smarty $template
*/
public static function statusCheck($template) {
if (CRM_Core_Config::isUpgradeMode() || !CRM_Core_Permission::check('administer CiviCRM')) {
$permissions = [];
// Transitional arrangement until end of 2019, we have added a new permission
// view status checks and if CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS is not defined
// or defined as false fall back to standard administer CiviCRM permission
if (!CRM_Utils_Constant::value('CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS')) {
$permissions[] = ['view status checks', 'administer CiviCRM'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton This should handle the transition i would have thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 my understanding of @totten's proposal is this - #16482 - note that if we go that way the removing this permission (or any others) from Administer CiviCRM is not in the plan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton I was under the impression that it was all about removing permissions from administer civicrm so you could then more finer grained allow for access by specific permission be it system or data. I have had a client request this style of functionality and I'm not sure if we are going to resolve #16482 any time soon tbh and this does implement the strategy you had requested in your previous comment of falling back by default to allowing for people with administer CiviCRM to see the status checks as they have before. I can certainly go either way but I guess i feel like we should be doing something more here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well when I thought through what I understood @totten to be suggesting it was NOT removing permissions from administer CiviCRM - but rather creating new permissions that would allow us not to give Administer CiviCRM to users unless we really wanted them to have 'root' permission

}
else {
$permissions[0] = 'view status checks';
}
if (CRM_Core_Config::isUpgradeMode() || !CRM_Core_Permission::check($permissions)) {
return;
}
// always use cached results - they will be refreshed by the session timer
Expand Down
4 changes: 4 additions & 0 deletions CRM/Core/Permission.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,10 @@ public static function getCorePermissions() {
$prefix . ts('send SMS'),
ts('Send an SMS'),
],
'view status checks' => [
$prefix . ts('View CiviCRM Status Checks'),
ts('View CiviCRM Status Checks'),
],
];

return $permissions;
Expand Down
10 changes: 6 additions & 4 deletions CRM/Upgrade/Incremental/php/FiveSixteen.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,12 @@ class CRM_Upgrade_Incremental_php_FiveSixteen extends CRM_Upgrade_Incremental_Ba
* @param null $currentVer
*/
public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL) {
// Example: Generate a pre-upgrade message.
// if ($rev == '5.12.34') {
// $preUpgradeMessage .= '<p>' . ts('A new permission, "%1", has been added. This permission is now used to control access to the Manage Tags screen.', array(1 => ts('manage tags'))) . '</p>';
// }
if ($rev == '5.16.alpha1') {
$preUpgradeMessage .= '<p>' . ts('A new permission, "%1", has been added. For the moment this new permission or %2 permission will be enough to see the status checks. This will change at the end of 2019 when the permissions %1 will be required to access the status checks. We recommend at least one user has that permission granted to them. If System Administrators want to switch to just using the new permission check to control access to the status checks. They can add a define in civicrm.settings.php setting CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS to be TRUE to disable the transitional arrangement .', [
1 => 'View CiviCRM Status Checks',
2 => 'Administer CiviCRM',
]) . '</p>';
}
}

/**
Expand Down
11 changes: 10 additions & 1 deletion CRM/Utils/Check.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,16 @@ public static function getSeverityList() {
* Display daily system status alerts (admin only).
*/
public function showPeriodicAlerts() {
if (CRM_Core_Permission::check('administer CiviCRM')) {
// Transitional arrangement until end of 2019, we have added a new permission
// view status checks and if CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS is not defined
// or defined as false fall back to standard administer CiviCRM permission
if (!CRM_Utils_Constant::value('CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS')) {
$permissions[] = ['view status checks', 'administer CiviCRM'];
}
else {
$permissions[0] = 'view status checks';
}
if (CRM_Core_Permission::check($permissions)) {
$session = CRM_Core_Session::singleton();
if ($session->timer('check_' . __CLASS__, self::CHECK_TIMER)) {

Expand Down
9 changes: 9 additions & 0 deletions templates/CRM/common/civicrm.settings.php.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,15 @@ define('CIVICRM_DEADLOCK_RETRIES', 3);
*/
// define('CIVICRM_BAO_CACHE_ADAPTER', 'CRM_Core_BAO_Cache_Psr16');

/**
* Specify if CiviCRM should not translate administer CiviCRM Permission
* to include view status checks permission.
*
* This is a transitional arrangement and if left undefined administer civicrm
* will be treated like view status checks permission
*/
// define('CIVICRM_DISABLE_TRANSITION_STATUS_CHECKS', TRUE);

if (CIVICRM_UF === 'UnitTests') {
if (!defined('CIVICRM_CONTAINER_CACHE')) define('CIVICRM_CONTAINER_CACHE', 'auto');
if (!defined('CIVICRM_MYSQL_STRICT')) define('CIVICRM_MYSQL_STRICT', true);
Expand Down