diff --git a/local/o365/classes/feature/usersync/main.php b/local/o365/classes/feature/usersync/main.php index 2eee727f5..2ec83bb13 100644 --- a/local/o365/classes/feature/usersync/main.php +++ b/local/o365/classes/feature/usersync/main.php @@ -274,13 +274,14 @@ protected function apply_photo( } if (!empty($muser->picture)) { - // User has no photo. Deleting previous profile photo. + // User had a photo — clear it. $fs = get_file_storage(); $fs->delete_area_files($context->id, 'user', 'icon'); $DB->set_field('user', 'picture', 0, ['id' => $muser->id]); + $result = true; + } else { + $result = false; } - - $result = false; } else { // Both get_photo() and get_photos_batch() guarantee that $photodata is valid // binary image data at this point (validated via is_valid_photo_binary() in @@ -294,10 +295,27 @@ protected function apply_photo( fwrite($fp, $photodata); fclose($fp); + // Capture the existing f1 file content hash before process_new_icon + // deletes it. process_new_icon always returns a new file ID even for + // identical image content, so $newpicture != $muser->picture is not a + // reliable content-change indicator — we must compare hashes instead. + $fs = get_file_storage(); + $existingfile = $fs->get_file($context->id, 'user', 'icon', 0, '/', 'f1.png') + ?: $fs->get_file($context->id, 'user', 'icon', 0, '/', 'f1.jpg'); + $existinghash = $existingfile ? $existingfile->get_contenthash() : null; + $newpicture = process_new_icon($context, 'user', 'icon', 0, $tempfile); - if ($newpicture != $muser->picture) { + + if ($newpicture) { + // Always update user.picture because process_new_icon deleted the old + // files — the old ID is now stale regardless of content equality. $DB->set_field('user', 'picture', $newpicture, ['id' => $muser->id]); - $result = true; + + // Determine whether content actually changed by comparing hashes. + $newfile = $fs->get_file($context->id, 'user', 'icon', 0, '/', 'f1.png') + ?: $fs->get_file($context->id, 'user', 'icon', 0, '/', 'f1.jpg'); + $newhash = $newfile ? $newfile->get_contenthash() : null; + $result = ($existinghash !== $newhash); } @unlink($tempfile); @@ -463,8 +481,8 @@ public function apply_photo_public( $photodata, ?int $appassignid = null, ?int $currentpicture = null - ) { - $this->apply_photo($muserid, $photodata, false, $appassignid, $currentpicture); + ): bool { + return $this->apply_photo($muserid, $photodata, false, $appassignid, $currentpicture); } /** diff --git a/local/o365/classes/rest/unified.php b/local/o365/classes/rest/unified.php index 08ed3879d..46a03ee07 100644 --- a/local/o365/classes/rest/unified.php +++ b/local/o365/classes/rest/unified.php @@ -2670,36 +2670,36 @@ public function get_timezone(string $upn) { * Get timezones for multiple users using batch requests. * Microsoft Graph allows up to 20 requests per batch. * - * @param array $upns Array of user principal names - * @return array Associative array with UPN as key and timezone data as value + * @param array $objectids Array of Azure AD object IDs (GUIDs) + * @return array Associative array with objectid as key and timezone data as value */ - public function get_timezones_batch(array $upns): array { - if (empty($upns)) { + public function get_timezones_batch(array $objectids): array { + if (empty($objectids)) { return []; } $results = []; - $chunks = array_chunk($upns, 20); // Max 20 requests per batch. + $chunks = array_chunk($objectids, 20); // Max 20 requests per batch. foreach ($chunks as $chunk) { $batchrequests = []; - $idtoupnmap = []; + $idtoobjectidmap = []; - // Pre-initialise every UPN to false so that UPNs absent from the batch - // response (partial API failure) are treated as "no timezone" rather than - // silently skipped by the caller's isset() / empty() branch logic. - foreach ($chunk as $upn) { - $results[$upn] = false; + // Pre-initialise every objectid to false so that objectids absent from + // the batch response (partial API failure) are treated as "no timezone" + // rather than silently skipped by the caller's isset() / empty() branch. + foreach ($chunk as $objectid) { + $results[$objectid] = false; } // Build batch request. - foreach ($chunk as $index => $upn) { + foreach ($chunk as $index => $objectid) { $requestid = (string)($index + 1); - $idtoupnmap[$requestid] = $upn; + $idtoobjectidmap[$requestid] = $objectid; $batchrequests[] = [ 'id' => $requestid, 'method' => 'GET', - 'url' => '/users/' . urlencode($upn) . '/mailboxSettings/timeZone', + 'url' => '/users/' . urlencode($objectid) . '/mailboxSettings/timeZone', ]; } @@ -2712,19 +2712,19 @@ public function get_timezones_batch(array $upns): array { if (!empty($batchresponse['responses']) && is_array($batchresponse['responses'])) { foreach ($batchresponse['responses'] as $individualresponse) { $requestid = $individualresponse['id']; - $upn = $idtoupnmap[$requestid]; + $objectid = $idtoobjectidmap[$requestid]; if ($individualresponse['status'] === 200 && !empty($individualresponse['body'])) { - $results[$upn] = $individualresponse['body']; + $results[$objectid] = $individualresponse['body']; } else { // Request failed or returned non-200 status. - $results[$upn] = false; + $results[$objectid] = false; } } } } catch (moodle_exception $e) { // Batch call itself failed; pre-initialisation above already set all - // UPNs in this chunk to false, so no additional work needed here. + // objectids in this chunk to false, so no additional work needed here. debugging('Batch timezone request failed: ' . $e->getMessage(), DEBUG_DEVELOPER); } } diff --git a/local/o365/classes/task/photoandtimezonesync.php b/local/o365/classes/task/photoandtimezonesync.php index b6586d20a..98dfa7ff1 100644 --- a/local/o365/classes/task/photoandtimezonesync.php +++ b/local/o365/classes/task/photoandtimezonesync.php @@ -141,20 +141,24 @@ public function execute() { $this->mtrace('Batch ' . ($batchnum + 1) . '/' . $numbatches . ' (offset: ' . $offset . ')...'); - // Fetch batch of users. LEFT JOIN with appassign to check if assigned, but only - // get the first appassign id per user to avoid duplicate rows. + // Azure AD Object ID (GUID) is used as the photo API identifier because + // o365name may contain a bare username on some installations, which Graph rejects + // with HTTP 400. A GUID is always a valid user identifier for the Graph API. + // local_o365_appassign.muserid has a UNIQUE index, so a direct LEFT JOIN is safe. $sql = "SELECT obj.moodleid AS muserid, - obj.o365name AS upn, + obj.objectid, u.username, u.picture AS currentpicture, u.timezone AS currenttimezone, - (SELECT id FROM {local_o365_appassign} - WHERE muserid = obj.moodleid LIMIT 1) AS appassignid + appassign.id AS appassignid FROM {local_o365_objects} obj JOIN {user} u ON u.id = obj.moodleid + LEFT JOIN {local_o365_appassign} appassign ON appassign.muserid = obj.moodleid WHERE obj.type = 'user' AND u.deleted = 0 AND u.suspended = 0 + AND obj.objectid IS NOT NULL + AND obj.objectid != '' AND obj.o365name IS NOT NULL AND obj.o365name != '' ORDER BY obj.moodleid"; @@ -169,29 +173,28 @@ public function execute() { $this->mtrace('Loaded ' . count($users) . ' users from database.', 1); // Separate users by what needs updating. - $upnsforphotosync = []; - $upnsfortzsync = []; + // Both photos and timezones use objectid (GUID) as the API identifier. + $objectidsforphotosync = []; + $objectidsfortzsync = []; foreach ($users as $user) { - // Sync photos for all users if photo sync is enabled. if ($photosyncenabled) { - $upnsforphotosync[] = $user->upn; + $objectidsforphotosync[] = $user->objectid; } - // Sync timezone if enabled. if ($tzsynceenabled) { - $upnsfortzsync[] = $user->upn; + $objectidsfortzsync[] = $user->objectid; } } if ($photosyncenabled) { - $this->mtrace('Users for photo sync: ' . count($upnsforphotosync), 1); - $totalusersforphotosync += count($upnsforphotosync); + $this->mtrace('Users for photo sync: ' . count($objectidsforphotosync), 1); + $totalusersforphotosync += count($objectidsforphotosync); } if ($tzsynceenabled) { - $this->mtrace('Users for timezone sync: ' . count($upnsfortzsync), 1); - $totalusersfortzsync += count($upnsfortzsync); + $this->mtrace('Users for timezone sync: ' . count($objectidsfortzsync), 1); + $totalusersfortzsync += count($objectidsfortzsync); } // Construct the API client once per batch. Both the timezone and photo @@ -199,21 +202,21 @@ public function execute() { $apiclient = null; // Batch fetch timezones for users that need it. - $timezonesbyupn = []; - if ($tzsynceenabled && !empty($upnsfortzsync) && !PHPUNIT_TEST && !defined('BEHAT_SITE_RUNNING')) { + $timezonesbyobjectid = []; + if ($tzsynceenabled && !empty($objectidsfortzsync) && !PHPUNIT_TEST && !defined('BEHAT_SITE_RUNNING')) { try { $this->mtrace('Fetching timezones...', 1); $apiclient = $usersync->construct_user_api(); - $timezonesbyupn = $apiclient->get_timezones_batch($upnsfortzsync); - $this->mtrace('Fetched ' . count(array_filter($timezonesbyupn)) . ' timezones from API.', 2); + $timezonesbyobjectid = $apiclient->get_timezones_batch($objectidsfortzsync); + $this->mtrace('Fetched ' . count(array_filter($timezonesbyobjectid)) . ' timezones from API.', 2); } catch (moodle_exception $e) { $this->mtrace('Error fetching timezones: ' . $e->getMessage(), 1); utils::debug($e->getMessage(), __METHOD__, $e); } } - // Batch fetch photos for all users. - $photosbyupn = []; + // Batch fetch photos for all users, keyed by objectid. + $photosbyobjectid = []; $photofetchstats = [ 'success' => 0, 'not_found' => 0, @@ -222,16 +225,16 @@ public function execute() { 'batch_error' => 0, ]; - if ($photosyncenabled && !empty($upnsforphotosync) && !PHPUNIT_TEST && !defined('BEHAT_SITE_RUNNING')) { + if ($photosyncenabled && !empty($objectidsforphotosync) && !PHPUNIT_TEST && !defined('BEHAT_SITE_RUNNING')) { try { $this->mtrace('Fetching photos...', 1); if ($apiclient === null) { $apiclient = $usersync->construct_user_api(); } - $photosbyupn = $apiclient->get_photos_batch($upnsforphotosync); + $photosbyobjectid = $apiclient->get_photos_batch($objectidsforphotosync); // Count results by status. - foreach ($photosbyupn as $response) { + foreach ($photosbyobjectid as $response) { if (is_array($response) && isset($response['status'])) { $status = $response['status']; if (isset($photofetchstats[$status])) { @@ -252,7 +255,7 @@ public function execute() { } if ($photofetchstats['batch_error'] > 0) { $this->mtrace('Missing from batch response: ' . $photofetchstats['batch_error'] . - ' (requested: ' . count($upnsforphotosync) . ')', 2); + ' (requested: ' . count($objectidsforphotosync) . ')', 2); } } catch (moodle_exception $e) { $this->mtrace('Error fetching photos: ' . $e->getMessage(), 1); @@ -264,57 +267,120 @@ public function execute() { $batchphotoschanged = 0; $batchtimezoneschanged = 0; - if ($photosyncenabled && !empty($photosbyupn)) { + if ($photosyncenabled && !empty($photosbyobjectid)) { $this->mtrace('Applying photos...', 1); + // Batch-fetch stored raw-photo hashes so we can skip process_new_icon + // entirely for users whose photo binary hasn't changed since last sync. + // Chunk to 1000 IDs per query to stay within SQL Server's 2100-parameter limit. + $userids = array_keys($users); + $storedphotohashes = []; + foreach (array_chunk($userids, 1000) as $chunkedids) { + [$insql, $inparams] = $DB->get_in_or_equal($chunkedids); + $chunk = $DB->get_records_sql( + "SELECT userid, value FROM {user_preferences} + WHERE userid $insql AND name = 'local_o365_photo_hash'", + $inparams + ); + $storedphotohashes += $chunk; + } + foreach ($users as $user) { // Apply photo if available, successful, or needs clearing. - if (isset($photosbyupn[$user->upn])) { - $response = $photosbyupn[$user->upn]; + if (isset($photosbyobjectid[$user->objectid])) { + $response = $photosbyobjectid[$user->objectid]; // Handle new format (status array). $shouldapply = false; $photodata = false; $logmessage = ''; + $incominghash = null; if (is_array($response) && isset($response['status'])) { $status = $response['status']; if ($status === 'success') { + $incominghash = sha1($response['data']); + $storedhash = isset($storedphotohashes[$user->muserid]) + ? $storedphotohashes[$user->muserid]->value + : null; + if ($storedhash === $incominghash) { + // Raw photo binary unchanged — skip image processing. + continue; + } $photodata = $response['data']; $shouldapply = true; $logmessage = 'Photo changed.'; } else if ($status === 'not_found') { + if (empty($user->currentpicture)) { + // Already has no photo — nothing to clear. + continue; + } $photodata = false; $shouldapply = true; $logmessage = 'Photo cleared (not in M365).'; } } else if ($response !== false) { // Fallback for old format compatibility. + $incominghash = sha1($response); + $storedhash = $storedphotohashes[$user->muserid]->value ?? null; + if ($storedhash === $incominghash) { + continue; + } $photodata = $response; $shouldapply = true; $logmessage = 'Photo changed.'; } if ($shouldapply) { + $applysucceeded = false; try { - $usersync->apply_photo_public( + $changed = $usersync->apply_photo_public( $user->muserid, $photodata, $user->appassignid ?? null, $user->currentpicture ?? null ); - $this->mtrace('User "' . $user->username . '": ' . $logmessage, 2); - $batchphotoschanged++; + if ($changed) { + $this->mtrace('User "' . $user->username . '": ' . $logmessage, 2); + $batchphotoschanged++; + } + $applysucceeded = true; } catch (moodle_exception $e) { $this->mtrace('User "' . $user->username . '": Error applying photo - ' . $e->getMessage(), 2); utils::debug($e->getMessage(), __METHOD__, $e); } + // Manage stored hash outside the try/catch. Direct DB write + // avoids Moodle's static preference cache accumulating across + // large batches; $storedphotohashes tells us whether to INSERT, + // UPDATE, or DELETE without an extra SELECT. + if ($applysucceeded && $incominghash !== null) { + // Photo was applied — store/update the hash for future runs. + if (isset($storedphotohashes[$user->muserid])) { + $DB->set_field('user_preferences', 'value', $incominghash, [ + 'userid' => $user->muserid, + 'name' => 'local_o365_photo_hash', + ]); + } else { + $DB->insert_record('user_preferences', (object)[ + 'userid' => $user->muserid, + 'name' => 'local_o365_photo_hash', + 'value' => $incominghash, + ]); + } + } else if ($applysucceeded && $changed && isset($storedphotohashes[$user->muserid])) { + // Photo was cleared — remove stale hash so a future re-upload + // with identical binary content is not skipped by the hash check. + $DB->delete_records('user_preferences', [ + 'userid' => $user->muserid, + 'name' => 'local_o365_photo_hash', + ]); + } } } } } - if ($tzsynceenabled && !empty($timezonesbyupn)) { + if ($tzsynceenabled && !empty($timezonesbyobjectid)) { $this->mtrace('Applying timezones...', 1); foreach ($users as $user) { @@ -323,11 +389,11 @@ public function execute() { // full user record when the value has not changed. Normalisation and the // Etc/GMT mapping live only in apply_timezone, eliminating the duplicate // logic that previously existed here. - if (isset($timezonesbyupn[$user->upn]) && $timezonesbyupn[$user->upn] !== false) { + if (isset($timezonesbyobjectid[$user->objectid]) && $timezonesbyobjectid[$user->objectid] !== false) { try { $result = $usersync->apply_timezone_public( $user->muserid, - $timezonesbyupn[$user->upn], + $timezonesbyobjectid[$user->objectid], $user->currenttimezone ); if ($result['changed']) { @@ -370,7 +436,8 @@ public function execute() { set_config('photosync_progress', json_encode($progressdata), 'local_o365'); // Free memory. - unset($users, $photosbyupn, $timezonesbyupn, $upnsforphotosync, $upnsfortzsync, $apiclient); + unset($users, $photosbyobjectid, $storedphotohashes, $timezonesbyobjectid); + unset($objectidsforphotosync, $objectidsfortzsync, $apiclient); gc_collect_cycles(); }