Delete CalDav and CardDav shares upon group deletion #57075
Conversation
max65482
requested review from
ArtificialOwl,
CarlSchwan,
come-nc and
icewind1991
and removed request for
a team
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
Looks correct to me but I’d like a review from someone with more knowledge of the dav application.
Signed-off-by: Maximilian Martin <[email protected]>
SebastianKrupinski
force-pushed
the
fix_dangling_group_shares
branch
from
534a7de to
1fdbc84
Compare
SebastianKrupinski
left a comment
SebastianKrupinski
left a comment
There was a problem hiding this comment.
Code looks good so far...
| public function postDeleteGroup(string $gid): void { | ||
| $this->calDav->deleteAllSharesByUser('principals/groups/' . $gid); | ||
| $this->cardDav->deleteAllSharesByUser('principals/groups/' . $gid); | ||
| } |
There was a problem hiding this comment.
This breaks for groups with special characters like spaces that get replaced with "+"... Tested with "Test Group A"...
$event->getGroup()->getGID() returns "Test Group A" but the actual uri is "Test+Group+A" so the deletion fails and the group is not deleted.
Also tested with a renamed group, started with "Test Group 2" then renamed to "Test Group B" although the label of the group is changed... the uri stays the same... and the group is not deleted.
There was a problem hiding this comment.
Good find!
Pushed a new commit. This should fix the first case.
I could not reproduce the second case. Can you check it again with a changed group name without spaces (you have both cases otherwise)?
Signed-off-by: Maximilian Martin <[email protected]>
3 participants
Summary
Unlike user shares, group shares were not deleted when the group was deleted. This lead to dangling group shares (calenders shared with groups that didn't exist anymore). This PR removes all group shares upon deletion.
Checklist
3. to review, feature component)stable32)