Merge pull request #1213 from France-ioi/delete_objects_permissions

groupRemoveChild & groupDelete: mark permissions for propagation properly, run only needed propagations

app/database/group_group_store.go

@@ -102,7 +102,7 @@ func (s *GroupGroupStore) DeleteRelation(parentGroupID, childGroupID int64, shou
 	if shouldDeleteChildGroup {
 		s.GroupGroups().deleteGroupAndOrphanedDescendants(childGroupID)
 	} else {
-		// delete the relation we are asked to delete (triggers will delete a lot from groups_ancestors and mark relations for propagation)
+		// delete the relation we are asked to delete (triggers will mark relations for propagation)
 		mustNotBeError(s.GroupGroups().Delete("parent_group_id = ? AND child_group_id = ?", parentGroupID, childGroupID).Error())
 
 		permissionsResult := s.PermissionsGranted().
@@ -134,10 +134,12 @@ func (s *GroupGroupStore) deleteGroupAndOrphanedDescendants(groupID int64) {
 		Pluck("groups.id", &candidatesForDeletion).Error())
 
 	// we delete groups_groups linked to groupID here in order to recalculate new ancestors correctly
-	mustNotBeError(s.deleteObjectsLinkedToGroups([]int64{groupID}).Error())
+	groupRelationsDeleted, permissionsDeleted := s.deleteObjectsLinkedToGroups([]int64{groupID})
 
 	// recalculate relations
-	s.GroupGroups().createNewAncestors()
+	if groupRelationsDeleted > 0 {
+		s.GroupGroups().createNewAncestors()
+	}
 
 	var idsToDelete []int64
 	// besides the group with id = groupID, we also want to delete its descendants
@@ -162,9 +164,9 @@ func (s *GroupGroupStore) deleteGroupAndOrphanedDescendants(groupID int64) {
 			Pluck("groups.id", &idsToDelete).Error())
 
 		if len(idsToDelete) > 0 {
-			deleteResult := s.deleteObjectsLinkedToGroups(idsToDelete)
-			mustNotBeError(deleteResult.Error())
-			if deleteResult.RowsAffected() > 0 {
+			groupRelationsDeleted, permissionsDeletedForDescendants := s.deleteObjectsLinkedToGroups(idsToDelete)
+			permissionsDeleted += permissionsDeletedForDescendants
+			if groupRelationsDeleted > 0 {
 				s.GroupGroups().createNewAncestors()
 			}
 		}
@@ -174,20 +176,23 @@ func (s *GroupGroupStore) deleteGroupAndOrphanedDescendants(groupID int64) {
 	// cascading deletes from many tables including groups_ancestors, groups_propagate, permission_granted & permissions_generated
 	mustNotBeError(s.Groups().Delete("id IN (?)", idsToDelete).Error())
 
-	s.SchedulePermissionsPropagation()
+	if permissionsDeleted > 0 {
+		s.SchedulePermissionsPropagation()
+	}
 }
 
-func (s *GroupGroupStore) deleteObjectsLinkedToGroups(groupIDs []int64) *DB {
-	return s.Exec(`
-		DELETE group_children, group_parents, filters
-		FROM `+"`groups`"+`
-		LEFT JOIN groups_groups AS group_children
-			ON group_children.parent_group_id = groups.id
-		LEFT JOIN groups_groups AS group_parents
-			ON group_parents.child_group_id = groups.id
-		LEFT JOIN filters
-			ON filters.group_id = groups.id
-		WHERE groups.id IN(?)`, groupIDs)
+func (s *GroupGroupStore) deleteObjectsLinkedToGroups(groupIDs []int64) (groupRelationsDeleted, permissionsDeleted int64) {
+	result := s.GroupGroups().Delete("parent_group_id IN(?)", groupIDs)
+	mustNotBeError(result.Error())
+	groupRelationsDeleted += result.RowsAffected()
+	result = s.GroupGroups().Delete("child_group_id IN(?)", groupIDs)
+	mustNotBeError(result.Error())
+	groupRelationsDeleted += result.RowsAffected()
+	mustNotBeError(s.Table("filters").Delete("group_id IN(?)", groupIDs).Error())
+	result = s.PermissionsGranted().Delete("source_group_id IN(?)", groupIDs)
+	mustNotBeError(result.Error())
+	permissionsDeleted = result.RowsAffected()
+	return groupRelationsDeleted, permissionsDeleted
 }
 
 // CreateNewAncestors creates ancestors for groups marked as 'todo' in `groups_propagate`.

app/database/group_store_integration_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/France-ioi/AlgoreaBackend/v2/app/database"
 	"github.com/France-ioi/AlgoreaBackend/v2/testhelpers"
@@ -357,7 +358,7 @@ func TestGroupStore_CheckIfEntryConditionsStillSatisfiedForAllActiveParticipatio
 	}
 }
 
-func Test_GroupStore_DeleteGroup(t *testing.T) {
+func TestGroupStore_DeleteGroup(t *testing.T) {
 	testoutput.SuppressIfPasses(t)
 
 	db := testhelpers.SetupDBWithFixtureString(`groups: [{id: 1234}]`)
@@ -373,6 +374,52 @@ func Test_GroupStore_DeleteGroup(t *testing.T) {
 	assert.Empty(t, ids)
 }
 
+func TestGroupStore_DeleteGroup_RecomputesAccess(t *testing.T) {
+	testoutput.SuppressIfPasses(t)
+
+	db := testhelpers.SetupDBWithFixtureString(`
+		groups: [{id: 1234}, {id: 1235}]
+		items: [{id: 10, default_language_tag: fr}]
+		permissions_granted:
+			- {group_id: 1234, item_id: 10, source_group_id: 1235, can_view: content}
+			- {group_id: 1234, item_id: 10, source_group_id: 1234, can_view: info}
+		permissions_generated: [{group_id: 1234, item_id: 10, can_view_generated: content}]`)
+	defer func() { _ = db.Close() }()
+
+	dataStore := database.NewDataStore(db).Groups()
+	require.NoError(t, dataStore.InTransaction(func(store *database.DataStore) error {
+		return store.Groups().DeleteGroup(1235)
+	}))
+	var newPermission string
+	require.NoError(t, dataStore.Permissions().Where("group_id = 1234 AND item_id = 10").
+		PluckFirst("can_view_generated", &newPermission).Error())
+	assert.Equal(t, "info", newPermission)
+}
+
+func TestGroupStore_DeleteGroup_RecomputesAccessForOrphanedSourceGroups(t *testing.T) {
+	testoutput.SuppressIfPasses(t)
+
+	db := testhelpers.SetupDBWithFixtureString(`
+		groups: [{id: 1234}, {id: 1235}, {id: 1236}]
+		groups_groups: [{parent_group_id: 1235, child_group_id: 1236}]
+		groups_ancestors: [{ancestor_group_id: 1235, child_group_id: 1236}]
+		items: [{id: 10, default_language_tag: fr}]
+		permissions_granted:
+			- {group_id: 1234, item_id: 10, source_group_id: 1236, can_view: content}
+			- {group_id: 1234, item_id: 10, source_group_id: 1234, can_view: info}
+		permissions_generated: [{group_id: 1234, item_id: 10, can_view_generated: content}]`)
+	defer func() { _ = db.Close() }()
+
+	dataStore := database.NewDataStore(db).Groups()
+	require.NoError(t, dataStore.InTransaction(func(store *database.DataStore) error {
+		return store.Groups().DeleteGroup(1235)
+	}))
+	var newPermission string
+	require.NoError(t, dataStore.Permissions().Where("group_id = 1234 AND item_id = 10").
+		PluckFirst("can_view_generated", &newPermission).Error())
+	assert.Equal(t, "info", newPermission)
+}
+
 func TestGroupStore_TriggerBeforeUpdate_RefusesToModifyType(t *testing.T) {
 	const expectedErrorMessage = "Error 1644: Unable to change groups.type from/to User/Team"