Skip to content
Snippets Groups Projects
Unverified Commit f41af578 authored by Abdul Wadood's avatar Abdul Wadood
Browse files

Restore scheduled deletions if the user loses group/project access

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 `DestroyService`s 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.

Changelog: changed
EE: true
parent d31de061
Branches andrey-remove-group-caching
No related tags found
No related merge requests found
Showing
with 393 additions and 94 deletions
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment