From 65abf2a37c721e77925ebef3fa85bda0d7f3c036 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 15 May 2018 12:25:04 +0800 Subject: [PATCH 1/4] MDL-62134 privacy: consistantly call components methods --- privacy/classes/manager.php | 80 ++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/privacy/classes/manager.php b/privacy/classes/manager.php index 65300686b84..c33a82b908d 100644 --- a/privacy/classes/manager.php +++ b/privacy/classes/manager.php @@ -23,7 +23,14 @@ */ namespace core_privacy; use core_privacy\local\metadata\collection; +use core_privacy\local\metadata\null_provider; +use core_privacy\local\request\context_aware_provider; use core_privacy\local\request\contextlist_collection; +use core_privacy\local\request\core_data_provider; +use core_privacy\local\request\core_user_data_provider; +use core_privacy\local\request\data_provider; +use core_privacy\local\request\user_preference_provider; +use \core_privacy\local\metadata\provider as metadata_provider; defined('MOODLE_INTERNAL') || die(); @@ -120,7 +127,7 @@ class manager { */ public function component_is_compliant(string $component) : bool { // Components which don't store user data need only implement the null_provider. - if ($this->component_implements($component, \core_privacy\local\metadata\null_provider::class)) { + if ($this->component_implements($component, null_provider::class)) { return true; } @@ -129,8 +136,8 @@ class manager { } // Components which store user data must implement the local\metadata\provider and the local\request\data_provider. - if ($this->component_implements($component, \core_privacy\local\metadata\provider::class) && - $this->component_implements($component, \core_privacy\local\request\data_provider::class)) { + if ($this->component_implements($component, metadata_provider::class) && + $this->component_implements($component, data_provider::class)) { return true; } @@ -144,8 +151,8 @@ class manager { * @return string The key to retrieve the language string for the null provider reason. */ public function get_null_provider_reason(string $component) : string { - if ($this->component_implements($component, \core_privacy\local\metadata\null_provider::class)) { - return $this->get_provider_classname($component)::get_reason(); + if ($this->component_implements($component, null_provider::class)) { + return static::component_class_callback($component, null_provider::class, 'get_reason', []); } else { throw new \coding_exception('Call to undefined method', 'Please only call this method on a null provider.'); } @@ -177,8 +184,10 @@ class manager { // Get the metadata, and put into an assoc array indexed by component name. $metadata = []; foreach ($this->get_component_list() as $component) { - if ($this->component_implements($component, \core_privacy\local\metadata\provider::class)) { - $metadata[$component] = $this->get_provider_classname($component)::get_metadata(new collection($component)); + $componentmetadata = static::component_class_callback($component, metadata_provider::class, + 'get_metadata', [new collection($component)]); + if ($componentmetadata !== null) { + $metadata[$component] = $componentmetadata; } } return $metadata; @@ -208,9 +217,9 @@ class manager { $a->progress++; $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); - if ($this->component_implements($component, \core_privacy\local\request\core_user_data_provider::class)) { - $contextlist = $this->get_provider_classname($component)::get_contexts_for_userid($userid); - } else { + $contextlist = static::component_class_callback($component, core_user_data_provider::class, + 'get_contexts_for_userid', [$userid]); + if ($contextlist === null) { $contextlist = new local\request\contextlist(); } @@ -265,13 +274,14 @@ class manager { $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); // Core user data providers. - if ($this->component_implements($component, \core_privacy\local\request\core_user_data_provider::class)) { + if ($this->component_implements($component, core_user_data_provider::class)) { if (count($approvedcontextlist)) { // This plugin has data it knows about. It is responsible for storing basic data about anything it is // told to export. - $this->get_provider_classname($component)::export_user_data($approvedcontextlist); + static::component_class_callback($component, core_user_data_provider::class, + 'export_user_data', [$approvedcontextlist]); } - } else if (!$this->component_implements($component, \core_privacy\local\request\context_aware_provider::class)) { + } else if (!$this->component_implements($component, context_aware_provider::class)) { // This plugin does not know that it has data - export the shared data it doesn't know about. local\request\helper::export_data_for_null_provider($approvedcontextlist); } @@ -290,15 +300,13 @@ class manager { $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); // Core user preference providers. - if ($this->component_implements($component, \core_privacy\local\request\user_preference_provider::class)) { - $this->get_provider_classname($component)::export_user_preferences($contextlistcollection->get_userid()); - } + static::component_class_callback($component, user_preference_provider::class, + 'export_user_preferences', [$contextlistcollection->get_userid()]); // Contextual information providers. Give each component a chance to include context information based on the // existence of a child context in the contextlist_collection. - if ($this->component_implements($component, \core_privacy\local\request\context_aware_provider::class)) { - $this->get_provider_classname($component)::export_context_data($contextlistcollection); - } + static::component_class_callback($component, context_aware_provider::class, + 'export_context_data', [$contextlistcollection]); } $progress->output(get_string('trace:done', 'core_privacy'), 1); @@ -343,12 +351,11 @@ class manager { $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); - if ($this->component_is_core_provider($component)) { - if (count($approvedcontextlist)) { - // The component knows about data that it has. - // Have it delete its own data. - $this->get_provider_classname($approvedcontextlist->get_component())::delete_data_for_user($approvedcontextlist); - } + if (count($approvedcontextlist)) { + // The component knows about data that it has. + // Have it delete its own data. + static::component_class_callback($approvedcontextlist->get_component(), core_user_data_provider::class, + 'delete_data_for_user', [$approvedcontextlist]); } // Delete any shared user data it doesn't know about. @@ -360,7 +367,7 @@ class manager { /** * Delete all use data which matches the specified deletion criteria. * - * @param context $context The specific context to delete data for. + * @param \context $context The specific context to delete data for. */ public function delete_data_for_all_users_in_context(\context $context) { $progress = static::get_log_tracer(); @@ -380,11 +387,10 @@ class manager { $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); - if ($this->component_implements($component, \core_privacy\local\request\core_user_data_provider::class)) { - // This component knows about specific data that it owns. - // Have it delete all of that user data for the context. - $this->get_provider_classname($component)::delete_data_for_all_users_in_context($context); - } + // If this component knows about specific data that it owns, + // have it delete all of that user data for the context. + static::component_class_callback($component, core_user_data_provider::class, + 'delete_data_for_all_users_in_context', [$context]); // Delete any shared user data it doesn't know about. local\request\helper::delete_data_for_all_users_in_context($component, $context); @@ -392,16 +398,6 @@ class manager { $progress->output(get_string('trace:done', 'core_privacy'), 1); } - /** - * Check whether the specified component is a core provider. - * - * @param string $component the frankenstyle component name. - * @return bool true if the component is a core provider, false otherwise. - */ - protected function component_is_core_provider($component) { - return $this->component_implements($component, \core_privacy\local\request\core_data_provider::class); - } - /** * Returns a list of frankenstyle names of core components (plugins and subsystems). * @@ -433,7 +429,7 @@ class manager { * @return string the fully qualified provider classname. */ public static function get_provider_classname_for_component(string $component) { - return "$component\privacy\provider"; + return "$component\\privacy\\provider"; } /** From 3f18d2af9a05317994ee5853229da238f43c6492 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 15 May 2018 12:33:47 +0800 Subject: [PATCH 2/4] MDL-62134 tool_dataprivacy: privacy manager wrapper If exception occurs in one plugin implementation do not fail the whole job but instead send a message to DPOs with the exception details --- .../classes/expired_contexts_manager.php | 2 +- admin/tool/dataprivacy/classes/manager.php | 97 +++++++++++++++++++ .../dataprivacy/classes/metadata_registry.php | 2 +- .../task/initiate_data_request_task.php | 3 +- .../task/process_data_request_task.php | 5 +- admin/tool/dataprivacy/db/messages.php | 8 ++ .../dataprivacy/lang/en/tool_dataprivacy.php | 3 + admin/tool/dataprivacy/version.php | 2 +- 8 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 admin/tool/dataprivacy/classes/manager.php diff --git a/admin/tool/dataprivacy/classes/expired_contexts_manager.php b/admin/tool/dataprivacy/classes/expired_contexts_manager.php index e71422df3b7..6c083ba3f5c 100644 --- a/admin/tool/dataprivacy/classes/expired_contexts_manager.php +++ b/admin/tool/dataprivacy/classes/expired_contexts_manager.php @@ -90,7 +90,7 @@ abstract class expired_contexts_manager { return $numprocessed; } - $privacymanager = new \core_privacy\manager(); + $privacymanager = new manager(); foreach ($this->get_context_levels() as $level) { diff --git a/admin/tool/dataprivacy/classes/manager.php b/admin/tool/dataprivacy/classes/manager.php new file mode 100644 index 00000000000..16c46a68fe7 --- /dev/null +++ b/admin/tool/dataprivacy/classes/manager.php @@ -0,0 +1,97 @@ +. +/** + * Class \tool_dataprivacy\manager + * + * @package tool_dataprivacy + * @copyright 2018 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tool_dataprivacy; +defined('MOODLE_INTERNAL') || die(); + +/** + * Wrapper for \core_privacy\manager that sends notifications about exceptions to DPO + * + * @package tool_dataprivacy + * @copyright 2018 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class manager extends \core_privacy\manager { + + /** + * Call the named method with the specified params on the supplied component if it implements the relevant interface on its provider. + * + * @param string $component The component to call + * @param string $interface The interface to implement + * @param string $methodname The method to call + * @param array $params The params to call + * @return mixed + */ + public static function component_class_callback(string $component, string $interface, string $methodname, array $params) { + try { + return parent::component_class_callback($component, $interface, $methodname, $params); + } catch (\Throwable $e) { + debugging($e->getMessage(), DEBUG_DEVELOPER, $e->getTrace()); + self::notify_dpo($e, $component, $interface, $methodname, $params); + } + return null; + } + + /** + * Notifies all DPOs about exception occurred + * + * @param \Throwable $e + * @param string $component + * @param string $interface + * @param string $methodname + * @param array $params + * @return mixed + */ + protected static function notify_dpo(\Throwable $e, string $component, string $interface, string $methodname, array $params) { + + // Get the list of the site Data Protection Officers. + $dpos = api::get_site_dpos(); + + $messagesubject = get_string('exceptionnotificationsubject', 'tool_dataprivacy'); + $a = (object)[ + 'fullmethodname' => static::get_provider_classname_for_component($component) . '::' . $methodname, + 'component' => $component, + 'message' => $e->getMessage(), + 'backtrace' => $e->getTraceAsString() + ]; + $messagebody = get_string('exceptionnotificationbody', 'tool_dataprivacy', $a); + + // Email the data request to the Data Protection Officer(s)/Admin(s). + foreach ($dpos as $dpo) { + $message = new \core\message\message(); + $message->courseid = SITEID; + $message->component = 'tool_dataprivacy'; + $message->name = 'notifyexceptions'; + $message->userfrom = \core_user::get_noreply_user(); + $message->subject = $messagesubject; + $message->fullmessageformat = FORMAT_HTML; + $message->notification = 1; + $message->userto = $dpo; + $message->fullmessagehtml = $messagebody; + $message->fullmessage = html_to_text($messagebody); + + // Send message. + return message_send($message); + } + } +} \ No newline at end of file diff --git a/admin/tool/dataprivacy/classes/metadata_registry.php b/admin/tool/dataprivacy/classes/metadata_registry.php index 7607643101a..b101f4238bd 100644 --- a/admin/tool/dataprivacy/classes/metadata_registry.php +++ b/admin/tool/dataprivacy/classes/metadata_registry.php @@ -39,7 +39,7 @@ class metadata_registry { * @return array An array with all of the plugin types / plugins and the user data they store. */ public function get_registry_metadata() { - $manager = new \core_privacy\manager(); + $manager = new manager(); $pluginman = \core_plugin_manager::instance(); $contributedplugins = $this->get_contrib_list(); $metadata = $manager->get_metadata_for_components(); diff --git a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php index 0cebeb0caaf..6554894beff 100644 --- a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php @@ -30,6 +30,7 @@ use moodle_exception; use tool_dataprivacy\api; use tool_dataprivacy\contextlist_context; use tool_dataprivacy\data_request; +use tool_dataprivacy\manager; defined('MOODLE_INTERNAL') || die(); @@ -96,7 +97,7 @@ class initiate_data_request_task extends adhoc_task { api::update_request_status($requestid, api::DATAREQUEST_STATUS_PREPROCESSING); // Add the list of relevant contexts to the request, and mark all as pending approval. - $privacymanager = new \core_privacy\manager(); + $privacymanager = new manager(); $contextlistcollection = $privacymanager->get_contexts_for_userid($datarequest->get('userid')); api::add_request_contexts_with_status($contextlistcollection, $requestid, contextlist_context::STATUS_PENDING); diff --git a/admin/tool/dataprivacy/classes/task/process_data_request_task.php b/admin/tool/dataprivacy/classes/task/process_data_request_task.php index 589ef01accd..2ef0b7d3685 100644 --- a/admin/tool/dataprivacy/classes/task/process_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/process_data_request_task.php @@ -33,6 +33,7 @@ use moodle_exception; use moodle_url; use tool_dataprivacy\api; use tool_dataprivacy\data_request; +use tool_dataprivacy\manager; defined('MOODLE_INTERNAL') || die(); @@ -87,7 +88,7 @@ class process_data_request_task extends adhoc_task { $approvedclcollection = api::get_approved_contextlist_collection_for_request($requestpersistent); // Export the data. - $manager = new \core_privacy\manager(); + $manager = new manager(); $exportedcontent = $manager->export_user_data($approvedclcollection); $fs = get_file_storage(); @@ -109,7 +110,7 @@ class process_data_request_task extends adhoc_task { $approvedclcollection = api::get_approved_contextlist_collection_for_request($requestpersistent); // Delete the data. - $manager = new \core_privacy\manager(); + $manager = new manager(); $manager->delete_data_for_user($approvedclcollection); } diff --git a/admin/tool/dataprivacy/db/messages.php b/admin/tool/dataprivacy/db/messages.php index 685d9b29ef0..2c1d239d4a0 100644 --- a/admin/tool/dataprivacy/db/messages.php +++ b/admin/tool/dataprivacy/db/messages.php @@ -41,4 +41,12 @@ $messageproviders = [ 'email' => MESSAGE_PERMITTED + MESSAGE_DEFAULT_LOGGEDIN + MESSAGE_DEFAULT_LOGGEDOFF, ] ], + + // Notify Data Protection Officer about exceptions. + 'notifyexceptions' => [ + 'defaults' => [ + 'email' => MESSAGE_PERMITTED + MESSAGE_DEFAULT_LOGGEDIN + MESSAGE_DEFAULT_LOGGEDOFF, + ], + 'capability' => 'tool/dataprivacy:managedatarequests' + ], ]; diff --git a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php index 8b089c7283f..40dc1e7cadb 100644 --- a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php +++ b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php @@ -101,6 +101,8 @@ $string['errorrequestalreadyexists'] = 'You already have an ongoing request.'; $string['errorrequestnotfound'] = 'Request not found'; $string['errorrequestnotwaitingforapproval'] = 'The request is not awaiting approval. Either it is not yet ready or it has already been processed.'; $string['errorsendingmessagetodpo'] = 'An error was encountered while trying to send a message to {$a}.'; +$string['exceptionnotificationsubject'] = "Exception occured while processing privacy data"; +$string['exceptionnotificationbody'] = "

Exception occured while calling {\$a->fullmethodname}.
This means that plugin {\$a->component} did not complete processing data. Below you can find exception information that can be passed to the plugin developer.

{\$a->message}
\n\n{\$a->backtrace}
"; $string['expiredretentionperiodtask'] = 'Expired retention period'; $string['expiry'] = 'Expiry'; $string['expandplugin'] = 'Expand and collapse plugin.'; @@ -148,6 +150,7 @@ $string['lawfulbases'] = 'Lawful bases'; $string['lawfulbases_help'] = 'Select at least one option that will serve as the lawful basis for processing personal data. For details on these lawful bases, please see GDPR Art. 6.1'; $string['messageprovider:contactdataprotectionofficer'] = 'Data requests'; $string['messageprovider:datarequestprocessingresults'] = 'Data request processing results'; +$string['messageprovider:notifyexceptions'] = 'Data requests exceptions notifications'; $string['message'] = 'Message'; $string['messagelabel'] = 'Message:'; $string['moduleinstancename'] = '{$a->instancename} ({$a->modulename})'; diff --git a/admin/tool/dataprivacy/version.php b/admin/tool/dataprivacy/version.php index 3561d57d518..7b7bde4be6e 100644 --- a/admin/tool/dataprivacy/version.php +++ b/admin/tool/dataprivacy/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die; -$plugin->version = 2018051400; +$plugin->version = 2018051401; $plugin->requires = 2018050800; // Moodle 3.5dev (Build 2018031600) and upwards. $plugin->component = 'tool_dataprivacy'; From 22c0a30888db4417b3b3fee420fe5f166fe90671 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 15 May 2018 14:16:54 +0800 Subject: [PATCH 3/4] MDL-62134 core_privacy: Allow for a failure handler --- privacy/classes/manager.php | 71 +++++++- privacy/classes/manager_observer.php | 47 +++++ privacy/tests/fixtures/provider_a.php | 91 ++++++++++ .../fixtures/provider_throwing_exception.php | 88 +++++++++ privacy/tests/manager_test.php | 167 +++++++++++++++++- 5 files changed, 453 insertions(+), 11 deletions(-) create mode 100644 privacy/classes/manager_observer.php create mode 100644 privacy/tests/fixtures/provider_a.php create mode 100644 privacy/tests/fixtures/provider_throwing_exception.php diff --git a/privacy/classes/manager.php b/privacy/classes/manager.php index c33a82b908d..47e73d291f9 100644 --- a/privacy/classes/manager.php +++ b/privacy/classes/manager.php @@ -26,7 +26,6 @@ use core_privacy\local\metadata\collection; use core_privacy\local\metadata\null_provider; use core_privacy\local\request\context_aware_provider; use core_privacy\local\request\contextlist_collection; -use core_privacy\local\request\core_data_provider; use core_privacy\local\request\core_user_data_provider; use core_privacy\local\request\data_provider; use core_privacy\local\request\user_preference_provider; @@ -110,6 +109,21 @@ defined('MOODLE_INTERNAL') || die(); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class manager { + + /** + * @var manager_observer Observer. + */ + protected $observer; + + /** + * Set the failure handler. + * + * @param manager_observer $observer + */ + public function set_observer(manager_observer $observer) { + $this->observer = $observer; + } + /** * Checks whether the given component is compliant with the core_privacy API. * To be considered compliant, a component must declare whether (and where) it stores personal data. @@ -152,7 +166,8 @@ class manager { */ public function get_null_provider_reason(string $component) : string { if ($this->component_implements($component, null_provider::class)) { - return static::component_class_callback($component, null_provider::class, 'get_reason', []); + $reason = $this->handled_component_class_callback($component, null_provider::class, 'get_reason', []); + return empty($reason) ? 'privacy:reason' : $reason; } else { throw new \coding_exception('Call to undefined method', 'Please only call this method on a null provider.'); } @@ -184,7 +199,7 @@ class manager { // Get the metadata, and put into an assoc array indexed by component name. $metadata = []; foreach ($this->get_component_list() as $component) { - $componentmetadata = static::component_class_callback($component, metadata_provider::class, + $componentmetadata = $this->handled_component_class_callback($component, metadata_provider::class, 'get_metadata', [new collection($component)]); if ($componentmetadata !== null) { $metadata[$component] = $componentmetadata; @@ -196,6 +211,7 @@ class manager { /** * Gets a collection of resultset objects for all components. * + * * @param int $userid the id of the user we're fetching contexts for. * @return contextlist_collection the collection of contextlist items for the respective components. */ @@ -217,7 +233,7 @@ class manager { $a->progress++; $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); - $contextlist = static::component_class_callback($component, core_user_data_provider::class, + $contextlist = $this->handled_component_class_callback($component, core_user_data_provider::class, 'get_contexts_for_userid', [$userid]); if ($contextlist === null) { $contextlist = new local\request\contextlist(); @@ -278,7 +294,7 @@ class manager { if (count($approvedcontextlist)) { // This plugin has data it knows about. It is responsible for storing basic data about anything it is // told to export. - static::component_class_callback($component, core_user_data_provider::class, + $this->handled_component_class_callback($component, core_user_data_provider::class, 'export_user_data', [$approvedcontextlist]); } } else if (!$this->component_implements($component, context_aware_provider::class)) { @@ -300,12 +316,12 @@ class manager { $a->datetime = userdate(time()); $progress->output(get_string('trace:processingcomponent', 'core_privacy', $a), 2); // Core user preference providers. - static::component_class_callback($component, user_preference_provider::class, + $this->handled_component_class_callback($component, user_preference_provider::class, 'export_user_preferences', [$contextlistcollection->get_userid()]); // Contextual information providers. Give each component a chance to include context information based on the // existence of a child context in the contextlist_collection. - static::component_class_callback($component, context_aware_provider::class, + $this->handled_component_class_callback($component, context_aware_provider::class, 'export_context_data', [$contextlistcollection]); } $progress->output(get_string('trace:done', 'core_privacy'), 1); @@ -354,7 +370,7 @@ class manager { if (count($approvedcontextlist)) { // The component knows about data that it has. // Have it delete its own data. - static::component_class_callback($approvedcontextlist->get_component(), core_user_data_provider::class, + $this->handled_component_class_callback($approvedcontextlist->get_component(), core_user_data_provider::class, 'delete_data_for_user', [$approvedcontextlist]); } @@ -389,7 +405,7 @@ class manager { // If this component knows about specific data that it owns, // have it delete all of that user data for the context. - static::component_class_callback($component, core_user_data_provider::class, + $this->handled_component_class_callback($component, core_user_data_provider::class, 'delete_data_for_all_users_in_context', [$context]); // Delete any shared user data it doesn't know about. @@ -496,4 +512,41 @@ class manager { return new \text_progress_trace(); } + + /** + * Call the named method with the specified params on the supplied component if it implements the relevant interface + * on its provider. + * + * @param string $component The component to call + * @param string $interface The interface to implement + * @param string $methodname The method to call + * @param array $params The params to call + * @return mixed + */ + protected function handled_component_class_callback(string $component, string $interface, string $methodname, array $params) { + try { + return static::component_class_callback($component, $interface, $methodname, $params); + } catch (\Throwable $e) { + debugging($e->getMessage(), DEBUG_DEVELOPER, $e->getTrace()); + $this->component_class_callback_failed($e, $component, $interface, $methodname, $params); + + return null; + } + } + + /** + * Notifies the observer of any failure. + * + * @param \Throwable $e + * @param string $component + * @param string $interface + * @param string $methodname + * @param array $params + */ + protected function component_class_callback_failed(\Throwable $e, string $component, string $interface, + string $methodname, array $params) { + if ($this->observer) { + call_user_func_array([$this->observer, 'handle_component_failure'], func_get_args()); + } + } } diff --git a/privacy/classes/manager_observer.php b/privacy/classes/manager_observer.php new file mode 100644 index 00000000000..b1d78477612 --- /dev/null +++ b/privacy/classes/manager_observer.php @@ -0,0 +1,47 @@ +. + +/** + * This file contains the interface required to observe failures in the manager. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +namespace core_privacy; + +defined('MOODLE_INTERNAL') || die(); + +/** + * The interface for a Manager observer. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +interface manager_observer { + + /** + * Handle failure of a component. + * + * @param \Throwable $e + * @param string $component + * @param string $interface + * @param string $methodname + * @param array $params + */ + public function handle_component_failure($e, $component, $interface, $methodname, array $params); +} diff --git a/privacy/tests/fixtures/provider_a.php b/privacy/tests/fixtures/provider_a.php new file mode 100644 index 00000000000..9f88acfbdfb --- /dev/null +++ b/privacy/tests/fixtures/provider_a.php @@ -0,0 +1,91 @@ +. + +/** + * Test provider which works. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace mod_component_a\privacy; + +defined('MOODLE_INTERNAL') || die(); + +use core_privacy\local\metadata\collection; +use core_privacy\local\request\contextlist; +use core_privacy\local\request\approved_contextlist; + +/** + * Mock core_user_data_provider for unit tests. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class provider implements + \core_privacy\local\metadata\provider, + \core_privacy\local\request\plugin\provider { + + /** + * Get metadata. + * + * @param collection $collection The initialised collection to add items to. + * @return collection A listing of user data stored through this system. + */ + public static function get_metadata(collection $collection) : collection { + $collection->add_subsystem_link('core_files'); + return $collection; + } + + /** + * Get the list of contexts that contain user information for the specified user. + * + * @param int $userid The user to search. + * @return contextlist $contextlist The contextlist containing the list of contexts used in this plugin. + */ + public static function get_contexts_for_userid(int $userid) : \core_privacy\local\request\contextlist { + $c = new \core_privacy\local\request\contextlist(); + $c->add_system_context(); + + return $c; + } + + /** + * Export all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts to export information for. + */ + public static function export_user_data(approved_contextlist $contextlist) { + } + + /** + * Delete all data for all users in the specified context. + * + * @param context $context The specific context to delete data for. + */ + public static function delete_data_for_all_users_in_context(\context $context) { + } + + /** + * Delete all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts and user information to delete information for. + */ + public static function delete_data_for_user(approved_contextlist $contextlist) { + } +} diff --git a/privacy/tests/fixtures/provider_throwing_exception.php b/privacy/tests/fixtures/provider_throwing_exception.php new file mode 100644 index 00000000000..6c6c5fc5efd --- /dev/null +++ b/privacy/tests/fixtures/provider_throwing_exception.php @@ -0,0 +1,88 @@ +. + +/** + * Test provider which has issues. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace mod_component_broken\privacy; + +use core_privacy\local\metadata\collection; +use core_privacy\local\request\contextlist; +use core_privacy\local\request\approved_contextlist; + +/** + * Mock core_user_data_provider for unit tests. + * + * @package core_privacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class provider implements + \core_privacy\local\metadata\provider, + \core_privacy\local\request\plugin\provider { + + /** + * Get metadata. + * + * @param collection $collection The initialised collection to add items to. + * @return collection A listing of user data stored through this system. + */ + public static function get_metadata(collection $collection) : collection { + throw new \coding_exception(__FUNCTION__); + } + + /** + * Get the list of contexts that contain user information for the specified user. + * + * @param int $userid The user to search. + * @return contextlist $contextlist The contextlist containing the list of contexts used in this plugin. + */ + public static function get_contexts_for_userid(int $userid) : \core_privacy\local\request\contextlist { + throw new \coding_exception(__FUNCTION__); + } + + /** + * Export all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts to export information for. + */ + public static function export_user_data(approved_contextlist $contextlist) { + throw new \coding_exception(__FUNCTION__); + } + + /** + * Delete all data for all users in the specified context. + * + * @param context $context The specific context to delete data for. + */ + public static function delete_data_for_all_users_in_context(\context $context) { + throw new \coding_exception(__FUNCTION__); + } + + /** + * Delete all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts and user information to delete information for. + */ + public static function delete_data_for_user(approved_contextlist $contextlist) { + throw new \coding_exception(__FUNCTION__); + } +} diff --git a/privacy/tests/manager_test.php b/privacy/tests/manager_test.php index b16e1c7aa9c..3f1f9acd55b 100644 --- a/privacy/tests/manager_test.php +++ b/privacy/tests/manager_test.php @@ -28,8 +28,11 @@ require_once($CFG->dirroot . '/privacy/tests/fixtures/mock_null_provider.php'); require_once($CFG->dirroot . '/privacy/tests/fixtures/mock_provider.php'); require_once($CFG->dirroot . '/privacy/tests/fixtures/mock_plugin_subplugin_provider.php'); require_once($CFG->dirroot . '/privacy/tests/fixtures/mock_mod_with_user_data_provider.php'); +require_once($CFG->dirroot . '/privacy/tests/fixtures/provider_a.php'); +require_once($CFG->dirroot . '/privacy/tests/fixtures/provider_throwing_exception.php'); use \core_privacy\local\request\writer; +use \core_privacy\local\request\approved_contextlist; /** * Privacy manager unit tests. @@ -175,7 +178,7 @@ class privacy_manager_testcase extends advanced_testcase { // Create an approved contextlist. $approvedcontextlistcollection = new \core_privacy\local\request\contextlist_collection(10); foreach ($contextlistcollection->get_contextlists() as $contextlist) { - $approvedcontextlist = new \core_privacy\local\request\approved_contextlist(new stdClass(), $contextlist->get_component(), + $approvedcontextlist = new approved_contextlist(new stdClass(), $contextlist->get_component(), $contextlist->get_contextids()); $approvedcontextlistcollection->add_contextlist($approvedcontextlist); } @@ -210,7 +213,7 @@ class privacy_manager_testcase extends advanced_testcase { // Create an approved contextlist. $approvedcontextlistcollection = new \core_privacy\local\request\contextlist_collection($user->id); foreach ($contextlistcollection->get_contextlists() as $contextlist) { - $approvedcontextlist = new \core_privacy\local\request\approved_contextlist($user, $contextlist->get_component(), + $approvedcontextlist = new approved_contextlist($user, $contextlist->get_component(), $contextlist->get_contextids()); $approvedcontextlistcollection->add_contextlist($approvedcontextlist); } @@ -300,4 +303,164 @@ class privacy_manager_testcase extends advanced_testcase { ], ]; } + + /** + * Test that get_contexts_for_userid() with a failing item. + */ + public function test_get_contexts_for_userid_with_failing() { + // Get a mock manager, in which the core components list is mocked to include all mock plugins. + // testcomponent is a core provider, testcomponent2 isa null provider, testcomponent3 is subplugin provider (non core). + $mockman = $this->get_mock_manager_with_core_components(['mod_component_broken', 'mod_component_a']); + + $observer = $this->getMockBuilder(\core_privacy\manager_observer::class) + ->setMethods(['handle_component_failure']) + ->getMock(); + $mockman->set_observer($observer); + + $observer->expects($this->once()) + ->method('handle_component_failure') + ->with( + $this->isInstanceOf(\coding_exception::class), + $this->identicalTo('mod_component_broken'), + $this->identicalTo(\core_privacy\local\request\core_user_data_provider::class), + $this->identicalTo('get_contexts_for_userid'), + $this->anything() + ); + + // Get the contextlist_collection. + $contextlistcollection = $mockman->get_contexts_for_userid(10); + $this->assertDebuggingCalled(); + $this->assertInstanceOf(\core_privacy\local\request\contextlist_collection::class, $contextlistcollection); + $this->assertCount(1, $contextlistcollection); + + // The component which completed shoudl have returned a contextlist. + $this->assertInstanceOf(\core_privacy\local\request\contextlist::class, + $contextlistcollection->get_contextlist_for_component('mod_component_a')); + $this->assertEmpty($contextlistcollection->get_contextlist_for_component('mod_component_broken')); + } + + /** + * Test that export_user_data() with a failing item. + */ + public function test_export_user_data_with_failing() { + $user = \core_user::get_user_by_username('admin'); + $mockman = $this->get_mock_manager_with_core_components(['mod_component_broken', 'mod_component_a']); + $context = \context_system::instance(); + $contextid = $context->id; + + $observer = $this->getMockBuilder(\core_privacy\manager_observer::class) + ->setMethods(['handle_component_failure']) + ->getMock(); + $mockman->set_observer($observer); + + $observer->expects($this->once()) + ->method('handle_component_failure') + ->with( + $this->isInstanceOf(\coding_exception::class), + $this->identicalTo('mod_component_broken'), + $this->identicalTo(\core_privacy\local\request\core_user_data_provider::class), + $this->identicalTo('export_user_data'), + $this->anything() + ); + + $collection = new \core_privacy\local\request\contextlist_collection(10); + $collection->add_contextlist(new approved_contextlist($user, 'mod_component_broken', [$contextid])); + $collection->add_contextlist(new approved_contextlist($user, 'mod_component_a', [$contextid])); + + // Get the contextlist_collection. + $mockman->export_user_data($collection); + $this->assertDebuggingCalled(); + } + + /** + * Test that delete_data_for_user() with a failing item. + */ + public function test_delete_data_for_user_with_failing() { + $user = \core_user::get_user_by_username('admin'); + $mockman = $this->get_mock_manager_with_core_components(['mod_component_broken', 'mod_component_a']); + $context = \context_system::instance(); + $contextid = $context->id; + + $observer = $this->getMockBuilder(\core_privacy\manager_observer::class) + ->setMethods(['handle_component_failure']) + ->getMock(); + $mockman->set_observer($observer); + + $observer->expects($this->once()) + ->method('handle_component_failure') + ->with( + $this->isInstanceOf(\coding_exception::class), + $this->identicalTo('mod_component_broken'), + $this->identicalTo(\core_privacy\local\request\core_user_data_provider::class), + $this->identicalTo('delete_data_for_user'), + $this->anything() + ); + + $collection = new \core_privacy\local\request\contextlist_collection(10); + $collection->add_contextlist(new approved_contextlist($user, 'mod_component_broken', [$contextid])); + $collection->add_contextlist(new approved_contextlist($user, 'mod_component_a', [$contextid])); + + // Get the contextlist_collection. + $mockman->delete_data_for_user($collection); + $this->assertDebuggingCalled(); + } + + /** + * Test that delete_data_for_all_users_in_context() with a failing item. + */ + public function test_delete_data_for_all_users_in_context_with_failing() { + $user = \core_user::get_user_by_username('admin'); + $mockman = $this->get_mock_manager_with_core_components(['mod_component_broken', 'mod_component_a']); + $context = \context_system::instance(); + + $observer = $this->getMockBuilder(\core_privacy\manager_observer::class) + ->setMethods(['handle_component_failure']) + ->getMock(); + $mockman->set_observer($observer); + + $observer->expects($this->once()) + ->method('handle_component_failure') + ->with( + $this->isInstanceOf(\coding_exception::class), + $this->identicalTo('mod_component_broken'), + $this->identicalTo(\core_privacy\local\request\core_user_data_provider::class), + $this->identicalTo('delete_data_for_all_users_in_context'), + $this->anything() + ); + + // Get the contextlist_collection. + $mockman->delete_data_for_all_users_in_context($context); + $this->assertDebuggingCalled(); + } + + /** + * Test that get_metadata_for_components() with a failing item. + */ + public function test_get_metadata_for_components_with_failing() { + $user = \core_user::get_user_by_username('admin'); + $mockman = $this->get_mock_manager_with_core_components(['mod_component_broken', 'mod_component_a']); + $context = \context_system::instance(); + + $observer = $this->getMockBuilder(\core_privacy\manager_observer::class) + ->setMethods(['handle_component_failure']) + ->getMock(); + $mockman->set_observer($observer); + + $observer->expects($this->once()) + ->method('handle_component_failure') + ->with( + $this->isInstanceOf(\coding_exception::class), + $this->identicalTo('mod_component_broken'), + $this->identicalTo(\core_privacy\local\metadata\provider::class), + $this->identicalTo('get_metadata'), + $this->anything() + ); + + // Get the contextlist_collection. + $metadata = $mockman->get_metadata_for_components(); + $this->assertDebuggingCalled(); + + $this->assertInternalType('array', $metadata); + $this->assertCount(1, $metadata); + } } From 8760b7335ba3532fb0fd869f93b47d98f1cc86e7 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 15 May 2018 14:17:18 +0800 Subject: [PATCH 4/4] MDL-62134 tool_dataprivacy: Add a manager_observer --- .../classes/expired_contexts_manager.php | 3 +- .../{manager.php => manager_observer.php} | 37 ++---- .../dataprivacy/classes/metadata_registry.php | 4 +- .../task/initiate_data_request_task.php | 4 +- .../task/process_data_request_task.php | 9 +- .../tests/manager_observer_test.php | 117 ++++++++++++++++++ 6 files changed, 139 insertions(+), 35 deletions(-) rename admin/tool/dataprivacy/classes/{manager.php => manager_observer.php} (64%) create mode 100644 admin/tool/dataprivacy/tests/manager_observer_test.php diff --git a/admin/tool/dataprivacy/classes/expired_contexts_manager.php b/admin/tool/dataprivacy/classes/expired_contexts_manager.php index 6c083ba3f5c..3d20e5d0cc6 100644 --- a/admin/tool/dataprivacy/classes/expired_contexts_manager.php +++ b/admin/tool/dataprivacy/classes/expired_contexts_manager.php @@ -90,7 +90,8 @@ abstract class expired_contexts_manager { return $numprocessed; } - $privacymanager = new manager(); + $privacymanager = new \core_privacy\manager(); + $privacymanager->set_observer(new \tool_dataprivacy\manager_observer()); foreach ($this->get_context_levels() as $level) { diff --git a/admin/tool/dataprivacy/classes/manager.php b/admin/tool/dataprivacy/classes/manager_observer.php similarity index 64% rename from admin/tool/dataprivacy/classes/manager.php rename to admin/tool/dataprivacy/classes/manager_observer.php index 16c46a68fe7..578a7d6ced4 100644 --- a/admin/tool/dataprivacy/classes/manager.php +++ b/admin/tool/dataprivacy/classes/manager_observer.php @@ -13,6 +13,7 @@ // // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . + /** * Class \tool_dataprivacy\manager * @@ -25,51 +26,29 @@ namespace tool_dataprivacy; defined('MOODLE_INTERNAL') || die(); /** - * Wrapper for \core_privacy\manager that sends notifications about exceptions to DPO + * A failure observer for the \core_privacy\manager. * * @package tool_dataprivacy * @copyright 2018 Marina Glancy * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class manager extends \core_privacy\manager { - +class manager_observer implements \core_privacy\manager_observer { /** - * Call the named method with the specified params on the supplied component if it implements the relevant interface on its provider. - * - * @param string $component The component to call - * @param string $interface The interface to implement - * @param string $methodname The method to call - * @param array $params The params to call - * @return mixed - */ - public static function component_class_callback(string $component, string $interface, string $methodname, array $params) { - try { - return parent::component_class_callback($component, $interface, $methodname, $params); - } catch (\Throwable $e) { - debugging($e->getMessage(), DEBUG_DEVELOPER, $e->getTrace()); - self::notify_dpo($e, $component, $interface, $methodname, $params); - } - return null; - } - - /** - * Notifies all DPOs about exception occurred + * Notifies all DPOs that an exception occurred. * * @param \Throwable $e * @param string $component * @param string $interface * @param string $methodname * @param array $params - * @return mixed */ - protected static function notify_dpo(\Throwable $e, string $component, string $interface, string $methodname, array $params) { - + public function handle_component_failure($e, $component, $interface, $methodname, array $params) { // Get the list of the site Data Protection Officers. $dpos = api::get_site_dpos(); $messagesubject = get_string('exceptionnotificationsubject', 'tool_dataprivacy'); $a = (object)[ - 'fullmethodname' => static::get_provider_classname_for_component($component) . '::' . $methodname, + 'fullmethodname' => \core_privacy\manager::get_provider_classname_for_component($component) . '::' . $methodname, 'component' => $component, 'message' => $e->getMessage(), 'backtrace' => $e->getTraceAsString() @@ -91,7 +70,7 @@ class manager extends \core_privacy\manager { $message->fullmessage = html_to_text($messagebody); // Send message. - return message_send($message); + message_send($message); } } -} \ No newline at end of file +} diff --git a/admin/tool/dataprivacy/classes/metadata_registry.php b/admin/tool/dataprivacy/classes/metadata_registry.php index b101f4238bd..6282cc41291 100644 --- a/admin/tool/dataprivacy/classes/metadata_registry.php +++ b/admin/tool/dataprivacy/classes/metadata_registry.php @@ -39,7 +39,9 @@ class metadata_registry { * @return array An array with all of the plugin types / plugins and the user data they store. */ public function get_registry_metadata() { - $manager = new manager(); + $manager = new \core_privacy\manager(); + $manager->set_observer(new \tool_dataprivacy\manager_observer()); + $pluginman = \core_plugin_manager::instance(); $contributedplugins = $this->get_contrib_list(); $metadata = $manager->get_metadata_for_components(); diff --git a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php index 6554894beff..ad5a933090e 100644 --- a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php @@ -97,7 +97,9 @@ class initiate_data_request_task extends adhoc_task { api::update_request_status($requestid, api::DATAREQUEST_STATUS_PREPROCESSING); // Add the list of relevant contexts to the request, and mark all as pending approval. - $privacymanager = new manager(); + $privacymanager = new \core_privacy\manager(); + $privacymanager->set_observer(new \tool_dataprivacy\manager_observer()); + $contextlistcollection = $privacymanager->get_contexts_for_userid($datarequest->get('userid')); api::add_request_contexts_with_status($contextlistcollection, $requestid, contextlist_context::STATUS_PENDING); diff --git a/admin/tool/dataprivacy/classes/task/process_data_request_task.php b/admin/tool/dataprivacy/classes/task/process_data_request_task.php index 2ef0b7d3685..6a93217b4cc 100644 --- a/admin/tool/dataprivacy/classes/task/process_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/process_data_request_task.php @@ -33,7 +33,6 @@ use moodle_exception; use moodle_url; use tool_dataprivacy\api; use tool_dataprivacy\data_request; -use tool_dataprivacy\manager; defined('MOODLE_INTERNAL') || die(); @@ -88,7 +87,9 @@ class process_data_request_task extends adhoc_task { $approvedclcollection = api::get_approved_contextlist_collection_for_request($requestpersistent); // Export the data. - $manager = new manager(); + $manager = new \core_privacy\manager(); + $manager->set_observer(new \tool_dataprivacy\manager_observer()); + $exportedcontent = $manager->export_user_data($approvedclcollection); $fs = get_file_storage(); @@ -110,7 +111,9 @@ class process_data_request_task extends adhoc_task { $approvedclcollection = api::get_approved_contextlist_collection_for_request($requestpersistent); // Delete the data. - $manager = new manager(); + $manager = new \core_privacy\manager(); + $manager->set_observer(new \tool_dataprivacy\manager_observer()); + $manager->delete_data_for_user($approvedclcollection); } diff --git a/admin/tool/dataprivacy/tests/manager_observer_test.php b/admin/tool/dataprivacy/tests/manager_observer_test.php new file mode 100644 index 00000000000..6c710277501 --- /dev/null +++ b/admin/tool/dataprivacy/tests/manager_observer_test.php @@ -0,0 +1,117 @@ +. + +/** + * Tests for the manager observer. + * + * @package tool_dataprivacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * API tests. + * + * @package tool_dataprivacy + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tool_dataprivacy_manager_observer_testcase extends advanced_testcase { + + /** + * Helper to set andn return two users who are DPOs. + */ + protected function setup_site_dpos() { + global $DB; + $this->resetAfterTest(); + + $generator = new testing_data_generator(); + $u1 = $this->getDataGenerator()->create_user(); + $u2 = $this->getDataGenerator()->create_user(); + + $context = context_system::instance(); + + // Give the manager role with the capability to manage data requests. + $managerroleid = $DB->get_field('role', 'id', array('shortname' => 'manager')); + assign_capability('tool/dataprivacy:managedatarequests', CAP_ALLOW, $managerroleid, $context->id, true); + + // Assign both users as manager. + role_assign($managerroleid, $u1->id, $context->id); + role_assign($managerroleid, $u2->id, $context->id); + + // Only map the manager role to the DPO role. + set_config('dporoles', $managerroleid, 'tool_dataprivacy'); + + return \tool_dataprivacy\api::get_site_dpos(); + } + + /** + * Ensure that when users are configured as DPO, they are sent an message upon failure. + */ + public function test_handle_component_failure() { + $this->resetAfterTest(); + + // Create another user who is not a DPO. + $this->getDataGenerator()->create_user(); + + // Create the DPOs. + $dpos = $this->setup_site_dpos(); + + $observer = new \tool_dataprivacy\manager_observer(); + + // Handle the failure, catching messages. + $mailsink = $this->redirectMessages(); + $mailsink->clear(); + $observer->handle_component_failure(new \Exception('error'), 'foo', 'bar', 'baz', ['foobarbaz', 'bum']); + + // Messages should be sent to both DPOs only. + $this->assertEquals(2, $mailsink->count()); + + $messages = $mailsink->get_messages(); + $messageusers = array_map(function($message) { + return $message->useridto; + }, $messages); + + $this->assertEquals(array_keys($dpos), $messageusers, '', 0.0, 0, true); + } + + /** + * Ensure that when no user is configured as DPO, the message is sent to admin instead. + */ + public function test_handle_component_failure_no_dpo() { + $this->resetAfterTest(); + + // Create another user who is not a DPO or admin. + $this->getDataGenerator()->create_user(); + + $observer = new \tool_dataprivacy\manager_observer(); + + $mailsink = $this->redirectMessages(); + $mailsink->clear(); + $observer->handle_component_failure(new \Exception('error'), 'foo', 'bar', 'baz', ['foobarbaz', 'bum']); + + // Messages should have been sent only to the admin. + $this->assertEquals(1, $mailsink->count()); + + $messages = $mailsink->get_messages(); + $message = reset($messages); + + $admin = \core_user::get_user_by_username('admin'); + $this->assertEquals($admin->id, $message->useridto); + } +}