diff --git a/lang/en/role.php b/lang/en/role.php index 3f68a78c701..af51354d812 100644 --- a/lang/en/role.php +++ b/lang/en/role.php @@ -387,6 +387,7 @@ $string['reportbuilder:edit'] = 'Edit your own custom reports'; $string['reportbuilder:editall'] = 'Edit all custom reports'; $string['reportbuilder:scheduleviewas'] = 'Schedule reports to be viewed as other users'; $string['reportbuilder:view'] = 'View custom reports'; +$string['reportbuilder:viewall'] = 'View all custom reports'; $string['resetrole'] = 'Reset'; $string['resettingrole'] = 'Resetting role \'{$a}\''; $string['restore:configure'] = 'Configure restore options'; diff --git a/lib/db/access.php b/lib/db/access.php index 0ff9dddbd28..69b687aef83 100644 --- a/lib/db/access.php +++ b/lib/db/access.php @@ -2686,6 +2686,13 @@ $capabilities = array( ], ], + // Allow users to view all custom reports. + 'moodle/reportbuilder:viewall' => [ + 'captype' => 'read', + 'contextlevel' => CONTEXT_SYSTEM, + 'archetypes' => [], + ], + // Allow users to create/edit their own custom reports. 'moodle/reportbuilder:edit' => [ 'captype' => 'write', diff --git a/reportbuilder/classes/local/helpers/audience.php b/reportbuilder/classes/local/helpers/audience.php index 349cc4c2fe0..d3349137ff6 100644 --- a/reportbuilder/classes/local/helpers/audience.php +++ b/reportbuilder/classes/local/helpers/audience.php @@ -159,11 +159,11 @@ class audience { /** * Returns SQL to limit the list of reports to those that the given user has access to * - * - A user with 'editall' capability will have access to all reports + * - A user with 'viewall/editall' capability will have access to all reports * - A user with 'edit' capability will have access to: * - Those reports this user has created * - Those reports this user is in audience of - * - A user with 'view' capability will have access to: + * - Otherwise: * - Those reports this user is in audience of * * @param string $reporttablealias @@ -182,31 +182,29 @@ class audience { $context = context_system::instance(); } - // If user can't view all reports, limit the returned list to those reports they can see. - if (!has_capability('moodle/reportbuilder:editall', $context, $userid)) { - - // Select all reports accessible to the user based on audience. - [$reportselect, $params] = $DB->get_in_or_equal( - self::user_reports_list($userid), - SQL_PARAMS_NAMED, - database::generate_param_name('_'), - true, - null, - ); - - $where = "{$reporttablealias}.id {$reportselect}"; - - // User can also see any reports that they can edit. - if (has_capability('moodle/reportbuilder:edit', $context, $userid)) { - $paramuserid = database::generate_param_name(); - $where = "({$reporttablealias}.usercreated = :{$paramuserid} OR {$where})"; - $params[$paramuserid] = $userid ?? $USER->id; - } - - return [$where, $params]; + if (has_any_capability(['moodle/reportbuilder:editall', 'moodle/reportbuilder:viewall'], $context, $userid)) { + return ['1=1', []]; } - return ['1=1', []]; + // Limit the returned list to those reports the user can see, by selecting based on report audience. + [$reportselect, $params] = $DB->get_in_or_equal( + self::user_reports_list($userid), + SQL_PARAMS_NAMED, + database::generate_param_name('_'), + true, + null, + ); + + $where = "{$reporttablealias}.id {$reportselect}"; + + // User can also see any reports that they can edit. + if (has_capability('moodle/reportbuilder:edit', $context, $userid)) { + $paramuserid = database::generate_param_name(); + $where = "({$reporttablealias}.usercreated = :{$paramuserid} OR {$where})"; + $params[$paramuserid] = $userid ?? $USER->id; + } + + return [$where, $params]; } /** diff --git a/reportbuilder/classes/permission.php b/reportbuilder/classes/permission.php index 38b6df30c3e..edf588c24cd 100644 --- a/reportbuilder/classes/permission.php +++ b/reportbuilder/classes/permission.php @@ -61,9 +61,10 @@ class permission { } return !empty($CFG->enablecustomreports) && has_any_capability([ - 'moodle/reportbuilder:editall', 'moodle/reportbuilder:edit', + 'moodle/reportbuilder:editall', 'moodle/reportbuilder:view', + 'moodle/reportbuilder:viewall', ], $context, $userid); } @@ -92,6 +93,10 @@ class permission { return false; } + if (has_capability('moodle/reportbuilder:viewall', $report->get_context(), $userid)) { + return true; + } + if (self::can_edit_report($report, $userid)) { return true; } diff --git a/reportbuilder/tests/local/helpers/audience_test.php b/reportbuilder/tests/local/helpers/audience_test.php index d0fa0069e7a..fcd20a33e96 100644 --- a/reportbuilder/tests/local/helpers/audience_test.php +++ b/reportbuilder/tests/local/helpers/audience_test.php @@ -267,16 +267,6 @@ class audience_test extends advanced_testcase { $this->setUser($usertwo); $usertworeport = $generator->create_report(['name' => 'User two report', 'source' => users::class]); - // Admin user sees all reports. - $this->setAdminUser(); - [$where, $params] = audience::user_reports_list_access_sql('r'); - $reports = $DB->get_fieldset_sql("SELECT r.id FROM {reportbuilder_report} r WHERE {$where}", $params); - $this->assertEqualsCanonicalizing([ - $useradminreport->get('id'), - $useronereport->get('id'), - $usertworeport->get('id'), - ], $reports); - // User one sees only the report they created. [$where, $params] = audience::user_reports_list_access_sql('r', (int) $userone->id); $reports = $DB->get_fieldset_sql("SELECT r.id FROM {reportbuilder_report} r WHERE {$where}", $params); @@ -298,6 +288,49 @@ class audience_test extends advanced_testcase { $this->assertEmpty($reports); } + /** + * Data provider for {@see test_user_reports_list_access_sql_with_capability} + * + * @return array[] + */ + public static function user_reports_list_access_sql_with_capability_provider(): array { + return [ + ['moodle/reportbuilder:editall'], + ['moodle/reportbuilder:viewall'], + ]; + } + + /** + * Test retrieving list of reports that user can access observes capability to view all reports + * + * @param string $capability + * + * @dataProvider user_reports_list_access_sql_with_capability_provider + */ + public function test_user_reports_list_access_sql_with_capability(string $capability): void { + global $DB; + + $this->resetAfterTest(); + + // Admin creates a report, no audience. + $this->setAdminUser(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Admin report', 'source' => users::class]); + + // Switch to new user, assign capability. + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $userrole = $DB->get_field('role', 'id', ['shortname' => 'user']); + assign_capability($capability, CAP_ALLOW, $userrole, context_system::instance()); + + [$where, $params] = audience::user_reports_list_access_sql('r'); + $reports = $DB->get_fieldset_sql("SELECT r.id FROM {reportbuilder_report} r WHERE {$where}", $params); + $this->assertEquals([$report->get('id')], $reports); + } + /** * Test getting list of audiences in use within schedules for a report */ diff --git a/reportbuilder/tests/permission_test.php b/reportbuilder/tests/permission_test.php index 3cbbaf24f82..f0806880b89 100644 --- a/reportbuilder/tests/permission_test.php +++ b/reportbuilder/tests/permission_test.php @@ -43,8 +43,10 @@ class permission_test extends advanced_testcase { $this->resetAfterTest(); - // User with permission. - $this->setAdminUser(); + // User with default permission. + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + try { permission::require_can_view_reports_list(); } catch (Throwable $exception) { @@ -52,9 +54,6 @@ class permission_test extends advanced_testcase { } // User without permission. - $user = $this->getDataGenerator()->create_user(); - $this->setUser($user); - $userrole = $DB->get_field('role', 'id', ['shortname' => 'user']); unassign_capability('moodle/reportbuilder:view', $userrole, context_system::instance()); @@ -63,6 +62,47 @@ class permission_test extends advanced_testcase { permission::require_can_view_reports_list(); } + /** + * Data provider for {@see test_require_can_view_reports_list_with_capability} + * + * @return array[] + */ + public static function require_can_view_reports_list_with_capability_provider(): array { + return [ + ['moodle/reportbuilder:edit'], + ['moodle/reportbuilder:editall'], + ['moodle/reportbuilder:viewall'], + ]; + } + + /** + * Test that viewing reports list observes capability to do so + * + * @param string $capability + * + * @dataProvider require_can_view_reports_list_with_capability_provider + */ + public function test_require_can_view_reports_list_with_capability(string $capability): void { + global $DB; + + $this->resetAfterTest(); + + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $userrole = $DB->get_field('role', 'id', ['shortname' => 'user']); + + // Remove default capability, allow additional. + unassign_capability('moodle/reportbuilder:view', $userrole, context_system::instance()); + assign_capability($capability, CAP_ALLOW, $userrole, context_system::instance()); + + try { + permission::require_can_view_reports_list(); + } catch (Throwable $exception) { + $this->fail($exception->getMessage()); + } + } + /** * Test whether user can view reports list when custom reports are disabled */ @@ -81,8 +121,6 @@ class permission_test extends advanced_testcase { * Test whether user can view specific report */ public function test_require_can_view_report(): void { - global $DB; - $this->resetAfterTest(); /** @var core_reportbuilder_generator $generator */ @@ -101,14 +139,56 @@ class permission_test extends advanced_testcase { $user = $this->getDataGenerator()->create_user(); $this->setUser($user); - $userrole = $DB->get_field('role', 'id', ['shortname' => 'user']); - unassign_capability('moodle/reportbuilder:view', $userrole, context_system::instance()); - $this->expectException(report_access_exception::class); $this->expectExceptionMessage('You cannot view this report'); permission::require_can_view_report($report); } + /** + * Data provider for {@see test_require_can_view_report_with_capability} + * + * @return array[] + */ + public static function require_can_view_report_with_capability_provider(): array { + return [ + ['moodle/reportbuilder:editall'], + ['moodle/reportbuilder:viewall'], + ]; + } + + /** + * Test whether user can view specific report when they have capability to view all reports + * + * @param string $capability + * + * @dataProvider require_can_view_report_with_capability_provider + */ + public function test_require_can_view_report_with_capability(string $capability): void { + global $DB; + + $this->resetAfterTest(); + + // Admin creates a report, no audience. + $this->setAdminUser(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Admin report', 'source' => users::class]); + + // Switch to new user, assign capability. + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $userrole = $DB->get_field('role', 'id', ['shortname' => 'user']); + assign_capability($capability, CAP_ALLOW, $userrole, context_system::instance()); + + try { + permission::require_can_view_report($report); + } catch (Throwable $exception) { + $this->fail($exception->getMessage()); + } + } + /** * Test whether user can view specific report when it belongs to an audience */ diff --git a/version.php b/version.php index d0a21e0c801..78955d46f22 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2024011900.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2024011900.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.4dev (Build: 20240119)'; // Human-friendly version name