MDL-77814 core_xapi: add itemid check to state store

The xAPI state standard allow any type of activity ID, not only
integers. However, the default state store uses itemid to identify the
component instance so the activity id is limited to numerics. The store
base calls should use string as activityid as this is how xAPI specs
describe it. However, if a plugin want to use non numeric activity ids
it must implement it's own state store.

lib/xapi/classes/state_store.php

@@ -40,6 +40,20 @@ class state_store {
         $this->component = $component;
     }
 
+    /**
+     * Convert the xAPI activity ID into an item ID integer.
+     *
+     * @throws xapi_exception if the activity id is not numeric.
+     * @param string $activityid the provided activity ID
+     * @return int
+     */
+    protected function activity_id_to_item_id(string $activityid): int {
+        if (!is_numeric($activityid)) {
+            throw new xapi_exception('The state store can only store numeric activity IDs.');
+        }
+        return intval($activityid);
+    }
+
     /**
      * Delete any extra state data stored in the database.
      *
@@ -55,7 +69,7 @@ class state_store {
         $data = [
             'component' => $this->component,
             'userid' => $state->get_user()->id,
-            'itemid' => $state->get_activity_id(),
+            'itemid' => $this->activity_id_to_item_id($state->get_activity_id()),
             'stateid' => $state->get_state_id(),
             'registration' => $state->get_registration(),
         ];
@@ -77,7 +91,7 @@ class state_store {
         $data = [
             'component' => $this->component,
             'userid' => $state->get_user()->id,
-            'itemid' => $state->get_activity_id(),
+            'itemid' => $this->activity_id_to_item_id($state->get_activity_id()),
             'stateid' => $state->get_state_id(),
             'registration' => $state->get_registration(),
         ];
@@ -109,7 +123,7 @@ class state_store {
         $data = [
             'component' => $this->component,
             'userid' => $state->get_user()->id,
-            'itemid' => $state->get_activity_id(),
+            'itemid' => $this->activity_id_to_item_id($state->get_activity_id()),
             'stateid' => $state->get_state_id(),
             'registration' => $state->get_registration(),
         ];
@@ -151,7 +165,7 @@ class state_store {
             'component' => $this->component,
         ];
         if ($itemid) {
-            $data['itemid'] = $itemid;
+            $data['itemid'] = $this->activity_id_to_item_id($itemid);
         }
         if ($userid) {
             $data['userid'] = $userid;
@@ -188,7 +202,7 @@ class state_store {
             'component' => $this->component,
         ];
         if ($itemid) {
-            $data['itemid'] = $itemid;
+            $data['itemid'] = $this->activity_id_to_item_id($itemid);
         }
         if ($userid) {
             $data['userid'] = $userid;
@@ -226,7 +240,7 @@ class state_store {
         ];
         if ($itemid) {
             $select .= ' AND itemid = :itemid';
-            $params['itemid'] = $itemid;
+            $params['itemid'] = $this->activity_id_to_item_id($itemid);
         }
         if ($userid) {
             $select .= ' AND userid = :userid';

lib/xapi/tests/state_store_test.php

@@ -554,4 +554,61 @@ class state_store_test extends advanced_testcase {
             ],
         ];
     }
+
+    /**
+     * Test delete with a non numeric activity id.
+     *
+     * The default state store only allows integer itemids.
+     *
+     * @dataProvider invalid_activityid_format_provider
+     * @param string $operation the method to execute
+     * @param bool $usestate if the param is a state or the activity id
+     */
+    public function test_invalid_activityid_format(string $operation, bool $usestate = false): void {
+        $this->resetAfterTest();
+        $this->setAdminUser();
+
+        $state = test_helper::create_state([
+            'activity' => item_activity::create_from_id('notnumeric'),
+        ]);
+        $param = ($usestate) ? $state : 'notnumeric';
+
+        $this->expectException(xapi_exception::class);
+        $store = new state_store('fake_component');
+        $store->$operation($param);
+    }
+
+    /**
+     * Data provider for test_invalid_activityid_format.
+     *
+     * @return array
+     */
+    public function invalid_activityid_format_provider(): array {
+        return [
+            'delete' => [
+                'operation' => 'delete',
+                'usestate' => true,
+            ],
+            'get' => [
+                'operation' => 'get',
+                'usestate' => true,
+            ],
+            'put' => [
+                'operation' => 'put',
+                'usestate' => true,
+            ],
+            'reset' => [
+                'operation' => 'reset',
+                'usestate' => false,
+            ],
+            'wipe' => [
+                'operation' => 'wipe',
+                'usestate' => false,
+            ],
+            'get_state_ids' => [
+                'operation' => 'get_state_ids',
+                'usestate' => false,
+            ],
+        ];
+    }
 }

lib/xapi/upgrade.txt

@@ -1,6 +1,12 @@
 This files describes API changes in core_xapi libraries and APIs,
 information provided here is intended especially for developers.
 
+=== 4.3 ===
+* The default state store will throw and exception if the activity ID is not
+  a numeric value. This is to avoid problems with the database when using
+  the default state store. If a plugin requires a non numeric activity ID,
+  it must implement its own state store.
+
 === 4.2 ===
 
 * A new state store has been introduced. Now plugins can store state data