SyncServerII / Neebla

Private and Self-Owned Social Media
MIT License
1 stars 2 forks source link

The Nasty Issue #12

Closed crspybits closed 3 years ago

crspybits commented 3 years ago

Rod created a new album, and sent an invitation to us. When Dany redeemed it, it didn't show up. Investigating, I found that for this album on Dany's app:

Album: sharingGroupUUID: B0D8A2A1-F476-4032-93CF-DED70D27CE3A; name: Optional("Interviewing"); deleted: true

I.e., the album was there but it is deleted. Dany didn't delete the album. There are only two places in Neebla where an album is deleted. And the only applicable one is this segment of code:

    // If `contentsSummary` is present in SharingGroup's, they will be updated.
    static func upsertSharingGroups(db: Connection, sharingGroups: [iOSBasics.SharingGroup]) throws {

        // First, need to deal with case of albums that we have locally but which are not listed on server. Those have been deleted.
        let localAlbums = try AlbumModel.fetch(db: db)

        for localAlbum in localAlbums {
            // Is this local album on the server?
            let onServer = sharingGroups.filter {$0.sharingGroupUUID == localAlbum.sharingGroupUUID}.count == 1
            if !onServer {
                logger.notice("Album was not on server; removing it locally.")
                // Not on server: Remove it locally.
                try localAlbum.update(setters: AlbumModel.deletedField.description <- true)
                try albumDeletionCleanup(db: db, sharingGroupUUID: localAlbum.sharingGroupUUID)
            }
        }

        // Second, update albums on the basis of the sharing groups.
        for sharingGroup in sharingGroups {
            try upsertSharingGroup(db: db, sharingGroup: sharingGroup)
        }
    }

There are multiple places that can call this method in the code, after doing a sync/index. What I think happened was the following:

1) A request was triggered to do an index/sync 2) Dany redeemed the sharing invitation. When these are successful, they automatically trigger another index/sync to update the list of sharing groups/albums.

Then due to a race condition 2), finishes first. So, the local list of albums/sharing groups is updated. Then 1) completes, but it has data reflecting the prior state of the albums. i.e., without the addition of the newly redeemed album. Then, upsertSharingGroups is called above. Because of the way that is structured right now, it doesn't find the sharing group in data from the server but does find it locally. It deletes the album.

This needs to be changed so that deletions occur only in a positive manner. i.e., only when a sharing group is explicitly marked as deleted will it be removed locally.

crspybits commented 3 years ago

1) [DONE] On the server, when removing a user from a sharing group, need to mark the user as removed, not actually remove a row.

2) [DONE] When sending notifications, don't send to users that have been removed from sharing groups.

3) [DONE] SharingGroup/SharingGroupUser security check needs to take into account the new deleted flag. See sharingGroupSecurityCheck.

4) [DONE] Revise tests for removeUserFromSharingGroup-- because it now just marks the user as having been removed from SharingGroupUser.

5) [DONE] I want to be able to re-add a user to a sharing group after they have been removed. Test will look like this: a) redeem invite to sharing group, b) add a file to that sharing group, c) remove yourself from the sharing group, d) redeem another invite to that sharing group, e) make sure there are no files present in the sharing group, f) add a file to the sharing group.

6) Test case: a) Redeem an invite, b) remove that sharing group, c) Try to access an endpoint using that sharing group, and ensure it fails.

Need to run a full set of server tests.

crspybits commented 3 years ago

1) Run a test of 5) above from Neebla. Re-add a user to a sharing group after they have been removed

crspybits commented 3 years ago

iOSBasics tests:

  1. Delete sharing group. Make sure that the sharing group is marked as deleted.

  2. In set case(s) Make sure given 1), SharingEntry's are marked as deleted.