Skip to content

Restore scheduled deletions if the user loses group/project access

Abdul Wadood requested to merge 39204-unschedule-project-deletion-on-user-delete into master

What does this MR do and why?

This change addresses an issue where users can schedule a group or project for deletion and subsequently lose access to it. This can occur in various scenarios, such as user deletion, membership downgrade, blocking, or banning.

Currently, when Sidekiq cron workers attempt to load groups/projects scheduled for deletion, the Sidekiq jobs fail indefinitely. This happens because the deleting user no longer has access to the group/project being deleted, as the DestroyServices include permission checks.

I considered several solutions:

  1. Skip the authorization check when calling the destroy services:

    • Pro: Simple implementation
    • Con: Highly destructive; risky if a bad actor schedules a group/project for deletion

  2. Delete the project/group deletion schedule when the user loses access:

    • Pro: Removes the deletion schedule immediately upon user removal
    • Con: Complex implementation arises due to numerous ways users can lose access, including deletion, downgrade, blocking, and banning. Handling all cases, especially membership downgrades through direct, inherited, or shared membership removal, is challenging. This complexity makes processing in the DELETE request to the members' removal API inefficient. The Members::DestroyService still needs optimization due to timeout issues. While a separate worker could be used, it's not the most efficient solution and may cause race conditions when users are quickly removed and re-added, potentially leaving group/project schedules intact. Moreover, this approach doesn't address existing projects and groups that are failing deletion.

  3. Check permissions in the cron worker deleting the scheduled projects/groups:

    • Pro: Covers all cases without needing a separate worker
    • Con: Deletion may succeed if a user regains access just before the scheduled time

We chose option 3 as the best solution. Although there's a minor risk of deletion succeeding if a user regains access just before the scheduled time, this aligns with our documentation stating that the user should have access at the scheduled time for deletion to succeed.

Currently, we partially use option 2 for group schedule deletion upon membership removal. However, it only deletes the group deletion schedule if the user has direct membership to the group. For inherited or shared membership, the group deletion schedule remains intact. This new approach addresses these limitations and provides a more comprehensive solution.

This implementation checks user permissions during the cron job execution and restores the resource if the user doesn't have access, ensuring that scheduled deletions are handled appropriately.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

Prerequisites: Ensure you have a Premium license or above

  1. Create a test group

    • Use user1 to create a new group
    • Add user2 to the group with owner access

  2. Schedule group for deletion

    • Login as user2
    • Schedule the group for deletion

  3. Update deletion schedule

    • Run the following in rails console: 
      group = Group.last
group.deletion_schedule.update(marked_for_deletion_on: 8.days.ago)

  4. Remove the user

    • Remove user2 from the test group

  5. Trigger restoration process

  6. Verify restoration

    • Check that the group has been restored instead of deleted

  7. Repeat the above process for project

    • Run the following in rails console: 
      project = Project.last
project.update(marked_for_deletion_at: 8.days.ago)

  8. Trigger restoration process

Related to #39204 #429890

Edited by Abdul Wadood

Merge request reports