Skip to content

refactor(dav): readability improvements #4225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshtrichards wants to merge 16 commits into
base: master
Choose a base branch
from
Open

refactor(dav): readability improvements #4225

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bc89619
docs: Add docblock for GroupFolderNode class
joshtrichards Dec 16, 2025
fe0ec1a
refactor(dav): clean up GroupFoldersHome - add type hints, unify erro…
joshtrichards Dec 16, 2025
226d476
docs: docblock for getChildForPrincipal class
joshtrichards Dec 16, 2025
9ac4b85
refactor(dav): improve readability of PropFindPlugin
joshtrichards Dec 16, 2025
423bcc7
chore: Add TODOs for logging silent edge cases
joshtrichards Dec 17, 2025
0ad5a77
refactor(dav): Formatting and docs for ACLPlugin
joshtrichards Dec 17, 2025
e0c7a15
chore: set userFolder prop to readonly
joshtrichards Dec 17, 2025
32dd476
refactor(dav): make user readonly move to const in ACLPlugin
joshtrichards Dec 17, 2025
93e92e4
refactor(dav): ACLPlugin.php for improved readability
joshtrichards Dec 17, 2025
4a44b9e
refactor(dav): ACLPlugin variable naming
joshtrichards Dec 17, 2025
d3ef9db
refactor(dav): more ACLPlugin tidying for clarity (vars - WIP/fixup)
joshtrichards Dec 18, 2025
b718ba8
refactor(dav): Clarify ACL handling in ACLPlugin comments
joshtrichards Dec 19, 2025
cbfe5fd
refactor(dav): Move long callbacks to private methods for readability
joshtrichards Dec 19, 2025
6b76d84
refactor(DAV): var name + move pvts in GroupFoldersHome
joshtrichards Dec 19, 2025
a0c1dab
chore: enhance comments in ACLPlugin
joshtrichards Dec 24, 2025
7597263
fix: Update PHPDoc return type and parameter descriptions
joshtrichards Dec 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 87 additions & 45 deletions lib/DAV/ACLPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,16 @@ function () use ($fileInfo, $mount): ?array {
$path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/');

if ($this->isAdmin($this->user, $fileInfo->getPath())) {
$rules = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]);
$rules = $this->ruleManager->getAllRulesForPaths(
$mount->getNumericStorageId(),
[$path]
);
} else {
$rules = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), [$path]);
$rules = $this->ruleManager->getRulesForFilesByPath(
$this->user,
$mount->getNumericStorageId(),
[$path]
);
}

return array_pop($rules);
Expand All @@ -121,9 +128,16 @@ function () use ($fileInfo, $mount): array {
$parentPaths[] = $mount->getSourcePath();

if ($this->isAdmin($this->user, $fileInfo->getPath())) {
$rulesByPath = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), $parentPaths);
$rulesByPath = $this->ruleManager->getAllRulesForPaths(
$mount->getNumericStorageId(),
$parentPaths
);
} else {
$rulesByPath = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), $parentPaths);
$rulesByPath = $this->ruleManager->getRulesForFilesByPath(
$this->user,
$mount->getNumericStorageId(),
$parentPaths
);
}

$aclManager = $this->aclManagerFactory->getACLManager($this->user);
Expand All @@ -132,9 +146,11 @@ function () use ($fileInfo, $mount): array {
$inheritedPermissionsByMapping = [];
$inheritedMaskByMapping = [];
$mappings = [];

foreach ($rulesByPath as $rules) {
foreach ($rules as $rule) {
$mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId();

if (!isset($mappings[$mappingKey])) {
$mappings[$mappingKey] = $rule->getUserMapping();
}
Expand Down Expand Up @@ -195,9 +211,11 @@ function () use ($fileInfo): bool {
function () use ($mount): int {
// Happens when sharing with a remote instance
if ($this->user === null) {
return Constants::PERMISSION_ALL;
return Constants::PERMISSION_ALL; // ???
}
return $this->aclManagerFactory->getACLManager($this->user)->getBasePermission($mount->getFolderId());
return $this->aclManagerFactory
->getACLManager($this->user)
->getBasePermission($mount->getFolderId());
}
);
}
Expand Down Expand Up @@ -232,64 +250,82 @@ public function propPatch(string $path, PropPatch $propPatch): void {
// Mapping the old property to the new property.
$propPatch->handle(
self::ACL_LIST,
function (array $rawRules) use ($path): bool {
$node = $this->server->tree->getNodeForPath($path);
if (!$node instanceof Node) {
return false;
}

$fileInfo = $node->getFileInfo();
$mount = $fileInfo->getMountPoint();
if (!$mount instanceof GroupMountPoint) {
return false;
}

if ($this->user === null) {
return false;
}
Copy link
Copy Markdown
Contributor

@come-nc come-nc Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all this removed?

function (array $rawRules) use ($node, $fileInfo, $mount, $path): bool {

$path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/');

// populate fileid in rules
$rules = array_values(array_map(fn (Rule $rule): Rule => new Rule(
$rule->getUserMapping(),
$fileInfo->getId(),
$rule->getMask(),
$rule->getPermissions()
), $rawRules));

$formattedRules = array_map(fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), $rules);
$rules = array_values(
array_map(
fn (Rule $rule): Rule => new Rule(
$rule->getUserMapping(),
$fileInfo->getId(),
$rule->getMask(),
$rule->getPermissions()
),
$rawRules
)
);

$formattedRules = array_map(
fn (Rule $rule): string =>
$rule->getUserMapping()->getType()
. ' '
. $rule->getUserMapping()->getDisplayName()
. ': ' . $rule->formatPermissions(),
$rules
);

if (count($formattedRules)) {
$formattedRules = implode(', ', $formattedRules);
$this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', [
$fileInfo->getInternalPath(),
$mount->getFolderId(),
$formattedRules,
]));
$this->eventDispatcher->dispatchTyped(
new CriticalActionPerformedEvent
(
'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"',
[ $fileInfo->getInternalPath(), $mount->getFolderId(), $formattedRules ]
)
);
} else {
$this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was cleared', [
$fileInfo->getInternalPath(),
$mount->getFolderId(),
]));
$this->eventDispatcher->dispatchTyped(
new CriticalActionPerformedEvent
(
'The advanced permissions for "%s" in Team folder with ID %d was cleared',
[ $fileInfo->getInternalPath(), $mount->getFolderId() ]
)
);
}

$aclManager = $this->aclManagerFactory->getACLManager($this->user);
$newPermissions = $aclManager->testACLPermissionsForPath($mount->getFolderId(), $mount->getNumericStorageId(), $path, $rules);
$newPermissions = $aclManager->testACLPermissionsForPath(
$mount->getFolderId(),
$mount->getNumericStorageId(),
$path,
$rules
);

if (!($newPermissions & Constants::PERMISSION_READ)) {
throw new BadRequest($this->l10n->t('You cannot remove your own read permission.'));
}

$existingRules = array_reduce(
$this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]),
$this->ruleManager->getAllRulesForPaths(
$mount->getNumericStorageId(),
[$path]
),
array_merge(...),
[]
);


$deletedRules = array_udiff($existingRules, $rules, fn (Rule $obj_a, Rule $obj_b): int => (
$obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType()
&& $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId()
) ? 0 : -1);
$deletedRules = array_udiff(
$existingRules,
$rules,
fn (Rule $obj_a, Rule $obj_b): int => (
$obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType()
&& $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId()
) ? 0 : -1
);

foreach ($deletedRules as $deletedRule) {
$this->ruleManager->deleteRule($deletedRule);
}
Expand All @@ -298,7 +334,13 @@ function (array $rawRules) use ($path): bool {
$this->ruleManager->saveRule($rule);
}

$node->getNode()->getStorage()->getPropagator()->propagateChange($fileInfo->getInternalPath(), $fileInfo->getMtime());
$node->getNode()
->getStorage()
->getPropagator()
->propagateChange(
$fileInfo->getInternalPath(),
$fileInfo->getMtime()
);

return true;
}
Expand Down