Repository navigation
Conversation
User IDs are used as array keys for the recipient lists and the notification settings. PHP stores numeric string keys like '55' as integers, and array_merge() renumbers integer keys from 0. With activity_use_cached_mountpoints enabled, file activities on instances with numeric user IDs were written for the users 0, 1, 2, ... instead of the real recipients. The email and notification settings of users who favorited the file were lost the same way. Use array_replace() for these keyed merges and cast the keys back to strings before comparing them with the current user, so the actor gets the "self" subject on rename and move. Fixes nextcloud#3071 Signed-off-by: motey <bleimehl@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3071
Problem
FilesHookskeys the recipient lists (uid => path) and the email/notification settings (uid => value) by user ID and combines them witharray_merge(). PHP stores numeric string keys like'55'as integers, andarray_merge()renumbers integer keys from 0. On instances with numeric user IDs (e.g.user_oidcwith a numericsub):activity_use_cached_mountpoints=true, file activities (created, changed, deleted, restored, renamed, moved) are written for the users0,1,2, ... instead of the real recipients. If a user with such an ID exists, they receive the activities of shares they have no access to.activity_use_cached_mountpoints=false.Change
array_replace()instead ofarray_merge()for the keyed merges ingetFileChangeActivitySettings(),addNotificationsForFileAction(),fileMove(),fileRenaming()andfileMoving(). For string keys the result is the same as before (later values win), integer keys are kept.fileRenaming()andgenerateDelete/Add/MoveActivities(), asaddNotificationsForFileAction()already does. Otherwise$user === $this->currentUser->getUID()is never true for numeric IDs and the actor getsrenamed_by/moved_byinstead ofrenamed_self/moved_self.The
array_merge()calls on plain lists ($allUsers,$usersToCheck,$rules) are not affected and stay as they are.Tests
Added to
tests/FilesHooksTest.php, all with numeric user IDs (56is the actor):testAddNotificationsForFileActionNumericUserIds: recipients from the access list, with and without cached mounts, plus a favorite. Checks the user IDs, subjects and email/notification settings passed toaddNotificationsForUser().testFileRenamingNumericUserIdsWithMountCachetestFileMoveNumericUserIdsWithMountCache: cross-folder move with deleted, added and moved users.All new tests fail on master (activities go to
0,1,2,3) and pass with this change. The full unit suite passes locally (470 tests, SQLite, server master), and php-cs-fixer and psalm report no issues.The same code is in stable33, stable34 and stable35, so a backport would help the affected instances.
🤖 AI (if applicable)