Give partially and fully shared courses the same deletion behaviour

[canstudios-nicolaw]

Fixes #2271
Fixes #2266

[tomgreenfield]

Confirmed #2266 is fixed.

Is it correct that authenticated users (who haven't even create permissions) are allowed to delete courses?

[canstudios-nicolaw]

Hm it's a good point, probably not... I suppose before the only reason that they couldn't delete courses was because they couldn't have created them.

[taylortom]

Regarding authenticated users, they should only have read access to courses (see the role definition for more info).

[canstudios-nicolaw]

@tomgreenfield @taylortom

I've spent some time revisting the logic that existed before this PR:

if(!canDeleteAll && docs[0]._isShared && docs[0].createdBy != user._id) {

To clarify, the value of "canDeleteAll" for different user types is:
Authenticated User - false
Course Creator - false
Product Manager - false
Tenant Admin - true
Super Admin - true

The original logic would have prevented AU/PM/CC users from deleting shared courses which did not belong to them. It is not possible for an AU to have created a course and so this would have always prevented them from deleting courses.

Now we want to change the logic so that anyone who can create a course can also delete it if it is shared.

I have been playing around with something along the lines of:

var resource = permissions.buildResourceString(tenantId, '/api/content/course/*');
      permissions.hasPermission(user._id, 'delete', resource, function(error, canDeleteAll) {
        permissions.hasPermission(user._id, 'create', resource, function(error, canCreateAll) {
            const sharedWithList = docs[0]._shareWithUsers;
            const isPartiallyShared = sharedWithList && sharedWithList.length;
            // Prevent Authenticated Users & Product Managers from deleting anything
            // Prevent Course Creators and Admins from deleting private courses which are not owned by them
            if(!canDeleteAll && !canCreateAll ||
              canCreateAll && (!docs[0]._isShared || !isPartiallyShared) && docs[0].createdBy !== user._id) {
                return next(new ContentPermissionError());
            }
            // Courses use cascading delete
            async.eachSeries(docs, function(doc, cb) {
                self.destroyChildren(doc._id, '_courseId', cb);
            }, function (err) {
                ContentPlugin.prototype.destroy.call(self, search, true, next);
            });
        });
      });

Where canCreateAll has the following values:
Authenticated User - false
Course Creator - true
Product Manager - false
Tenant Admin - true
Super Admin - true

I haven't tested this at all yet just posting my progress in case anyone else has any other thoughts. I will try and have another look at this on monday. I think this logic is quite tricky to get right now that there are more options. Ideally we shouldn't be depending on a check like this - hopefully something for the refactor.

[canstudios-nicolaw]

Actually that doesn't take into account that admins should be able to delete all courses..

So I guess it would need to be something more like:

var resource = permissions.buildResourceString(tenantId, '/api/content/course/*');
      permissions.hasPermission(user._id, 'delete', resource, function(error, canDeleteAll) {
        permissions.hasPermission(user._id, 'create', resource, function(error, canCreateAll) {
            const sharedWithList = docs[0]._shareWithUsers;
            const isPartiallyShared = sharedWithList && sharedWithList.length;
            // Prevent Authenticated Users & Product Managers from deleting anything
            // Prevent Course Creators from deleting private courses which are not owned by them
            if(!canDeleteAll && !canCreateAll ||
              !canDeleteAll && canCreateAll && (!docs[0]._isShared || !isPartiallyShared) && docs[0].createdBy !== user._id) {
                return next(new ContentPermissionError());
            }
            // Courses use cascading delete
            async.eachSeries(docs, function(doc, cb) {
                self.destroyChildren(doc._id, '_courseId', cb);
            }, function (err) {
                ContentPlugin.prototype.destroy.call(self, search, true, next);
            });
        });
      });

[canstudios-nicolaw]

Discussed in stand up - could we possibly add delete permission to the course creator role so that canDeleteAll = true for course creators. Then revert this logic back to what it was before this PR?

[canstudios-nicolaw]

With these changes SA/TA/CC users can delete shared courses, AU/PM users cannot.