OpenConceptLab/ocl_issues#147: Users can view public collections that…

… are marked private if the user query is specified

integration-tests/package.json

@@ -7,6 +7,7 @@
   "devDependencies": {
     "@types/jest": "^24.0.15",
     "@types/node": "^12.6.8",
+    "faker": "^4.1.0",
     "jest": "^24.8.0",
     "node-fetch": "^2.6.0",
     "ts-jest": "^24.0.2",

integration-tests/src/api.ts

@@ -0,0 +1,28 @@
+import {put, get, post} from "./utils"
+import {VerboseContainer} from "./types";
+
+const common = {
+    container: {
+        list: type => async (ownerUrl: string, token: string=null, verbose: boolean=true): Promise<VerboseContainer[]> => (await get(`${ownerUrl}${type}/?verbose=${verbose}`, token)).json(),
+        new: type => async (ownerUrl: string, body:object, token:string): Promise<VerboseContainer[]> => (await post(`${ownerUrl}${type}/`, body, token)).json(),
+    },
+};
+
+const api = {
+    organizations: {
+        new: async (orgId: string, token: string, publicAccess: boolean= true): Promise<Response> => publicAccess ?
+            post('orgs/', {id: orgId, name: orgId}, token) :
+            post('orgs/', {id: orgId, name: orgId, public_access: 'None'}, token),
+        addNewMember: async (membersUrl, user, token) => put(`${membersUrl}${user}/`, undefined, token),
+    },
+    collections: {
+        list: common.container.list('collections'),
+        new: common.container.new('collections'),
+    },
+    sources: {
+        list: common.container.list('sources'),
+        new: common.container.new('sources'),
+    },
+};
+
+export default api;

integration-tests/src/container.spec.ts

@@ -0,0 +1,155 @@
+import * as faker from 'faker';
+import {authenticate, authenticateAdmin, newUser, del} from "./utils";
+import containerFixture from "./fixtures/container";
+import api from "./api";
+
+const sortCriteria = (container1, container2) => {
+    if (container1.id < container2.id) return -1;
+    else if(container1.id > container2.id) return 1;
+    else return 0;
+};
+
+const cleanupAnyPublicContainers = async (adminToken, containerType) => {
+    const containers = await api[containerType].list('/');
+    for(let i in containers) await del(containers[i].url, adminToken);
+};
+
+describe.each([
+    'sources',
+    'collections',
+])('View Authorization Tests: %s', container => {
+    let adminToken;
+    let user1InOrg1;
+    let user2InOrg1;
+    let user3NotInOrg1;
+    let org1;
+    let privateContainerOwnedByUser1;
+    let privateContainerOwnedByOrg1;
+    let publicContainerOwnedByUser1;
+    let publicContainerOwnedByOrg1;
+
+    beforeAll(async () => {
+        adminToken = await authenticateAdmin();
+        // in case an afterAll is not executed, public containers would not be namespaced
+        // and would interfere with the public access tests
+        await cleanupAnyPublicContainers(adminToken, container);
+
+        // making this as random as possible since we can't delete users
+        const generateRandomName = () => faker.name.firstName() + faker.name.lastName();
+        const username1 = generateRandomName();
+        const username2 = generateRandomName();
+        const username3 = generateRandomName();
+        const orgId1 = faker.company.bsNoun();
+
+        user1InOrg1 = await newUser(username1, username1, adminToken);
+        user2InOrg1 = await newUser(username2, username2, adminToken);
+        user3NotInOrg1 = await newUser(username3, username3, adminToken);
+        user1InOrg1.token = await authenticate(username1, username1);
+        user2InOrg1.token = await authenticate(username2, username2);
+        user3NotInOrg1.token = await authenticate(username3, username3);
+
+        org1 = await (await api.organizations.new(orgId1, adminToken)).json();
+        await api.organizations.addNewMember(org1.members_url, user1InOrg1.username, adminToken);
+        await api.organizations.addNewMember(org1.members_url, user2InOrg1.username, adminToken);
+
+        privateContainerOwnedByUser1 = await api[container].new(user1InOrg1.url, containerFixture(), user1InOrg1.token);
+        privateContainerOwnedByOrg1 = await api[container].new(org1.url, containerFixture(), user1InOrg1.token);
+        publicContainerOwnedByUser1 = await api[container].new(user1InOrg1.url, containerFixture('View'), user1InOrg1.token);
+        publicContainerOwnedByOrg1 = await api[container].new(org1.url, containerFixture('View'), user1InOrg1.token);
+    });
+
+    afterAll(async () => {
+        const items = [
+            publicContainerOwnedByOrg1,
+            publicContainerOwnedByUser1,
+            privateContainerOwnedByOrg1,
+            privateContainerOwnedByUser1,
+            org1,
+            user3NotInOrg1,
+            user2InOrg1,
+            user1InOrg1,
+        ];
+        for(let i in items) await del(items[i].url, adminToken);
+    });
+
+    describe('logged in user', () => {
+        test(`can view own ${container}`, async () => {
+            const results = (await api[container].list(`${user1InOrg1.url}`, user1InOrg1.token)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByUser1, privateContainerOwnedByUser1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`can view their orgs ${container}`, async () => {
+            const results = (await api[container].list(`${org1.url}`, user2InOrg1.token)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByOrg1, privateContainerOwnedByOrg1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`can view another users public ${container}`, async () => {
+            const results = (await api[container].list(`${user1InOrg1.url}`, user2InOrg1.token)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByUser1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`can view another orgs public ${container}`, async () => {
+            const results = (await api[container].list(`${org1.url}`, user3NotInOrg1.token)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByOrg1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`cannot view another users private ${container}`, async () => {
+            const results = await api[container].list(`${user1InOrg1.url}`, user2InOrg1.token);
+            expect(results).not.toContain(privateContainerOwnedByUser1);
+        });
+
+        test(`cannot view another orgs private ${container}`, async () => {
+            const results = await api[container].list(`${org1.url}`, user3NotInOrg1.token);
+            expect(results).not.toContain(privateContainerOwnedByOrg1);
+        });
+    });
+
+    describe('not logged in user', () => {
+        test(`can view another users public ${container}`, async () => {
+            const results = (await api[container].list(`${user1InOrg1.url}`)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByUser1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`can view another orgs public ${container}`, async () => {
+            const results = (await api[container].list(`${org1.url}`)).sort(sortCriteria);
+            const expected = [publicContainerOwnedByOrg1].sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test(`cannot view another users private ${container}`, async () => {
+            const results = await api[container].list(`${user1InOrg1.url}`, user2InOrg1.token);
+            expect(results).not.toContain(privateContainerOwnedByUser1);
+        });
+
+        test(`cannot view another orgs private ${container}`, async () => {
+            const results = await api[container].list(`${org1.url}`);
+            expect(results).not.toContain(privateContainerOwnedByOrg1);
+        });
+    });
+
+    describe(`view all public ${container}`, () => {
+        test('logged in user', async () => {
+            const expected = [publicContainerOwnedByUser1, publicContainerOwnedByOrg1].sort(sortCriteria);
+
+            let results = (await api[container].list('/', user1InOrg1.token)).sort(sortCriteria);
+            expect(results).toEqual(expected);
+
+            results = (await api[container].list('/', user2InOrg1.token)).sort(sortCriteria);
+            expect(results).toEqual(expected);
+
+            results = (await api[container].list('/', user3NotInOrg1.token)).sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+
+        test('not logged in user', async () => {
+            const expected = [publicContainerOwnedByUser1, publicContainerOwnedByOrg1].sort(sortCriteria);
+            const results = (await api[container].list('/')).sort(sortCriteria);
+            expect(results).toEqual(expected);
+        });
+    });
+});

integration-tests/src/fixtures/container.ts

@@ -0,0 +1,21 @@
+import * as faker from 'faker';
+
+const container = (publicAccess = 'None') => {
+    return {
+        type: "Collection",
+        uuid: faker.random.uuid(),
+        id: faker.random.number(),
+        external_id: "",
+        short_code: faker.company.bsNoun(),
+        name: faker.company.companyName(),
+        full_name: faker.company.companyName(),
+        collection_type: "Core Dataset",
+        public_access: publicAccess,
+        supported_locales: "en,es",
+        website: "",
+        description: "",
+        extras: {},
+    }
+};
+
+export default container;

integration-tests/src/org.spec.ts

@@ -1,4 +1,5 @@
 import {get, post, del, newUser, authenticateAdmin, authenticate} from './utils';
+import api from "./api";
 
 describe('Org', () => {
     const uniqueId = '6a5bb179';
@@ -20,13 +21,7 @@ describe('Org', () => {
         return 'Test' + uniqueId + new Date().getTime() + 'Org';
     };
 
-    const newOrg = async (orgId: string, token: string, publicAccess: boolean= true): Promise<Response> => {
-        if (publicAccess) {
-            return post('orgs/', {id: orgId, name: orgId}, token);
-        } else {
-            return post('orgs/', {id: orgId, name: orgId, public_access: 'None'}, token);
-        }
-    };
+    const newOrg = api.organizations.new;
 
     const newCleanup = (url: string): (() => Promise<void>) => {
         return async () => {

integration-tests/src/types.ts

@@ -0,0 +1,20 @@
+export interface VerboseContainer {
+    type: string,
+    uuid: string,
+    id: string,
+    external_id: string,
+    short_code: string,
+    name: string,
+    full_name: string,
+    collection_type: string,
+    public_access: string,
+    supported_locales: string[],
+    website: string,
+    description: string,
+    extras: object,
+    url: string,
+    active_concepts: number,
+    active_mappings: number,
+    concepts_url: string,
+    created_by: string,
+}

integration-tests/src/utils.ts

@@ -21,8 +21,9 @@ export const joinUrl = function (url, part) {
 };
 
 export const newUser = async function(username, password, adminToken) {
-    await post('users/', {username: username, password: password, name: username, email: username + '@openconceptlab.org'}, adminToken);
+    const user = await post('users/', {username: username, password: password, name: username, email: username + '@openconceptlab.org'}, adminToken);
     await put('users/' + username + '/reactivate/', {username: username, password: password, name: username, email: username + '@openconceptlab.org'}, adminToken);
+    return await user.json();
 };
 
 export const post = async function(url, body, token=null) {

ocl/collection/filters.py

@@ -1,15 +1,7 @@
 __author__ = 'snyaggarwal'
 
-from oclapi.filters import HaystackSearchFilter
+from oclapi.filters import ConceptContainerPermissionedSearchFilter
 
 
-class CollectionSearchFilter(HaystackSearchFilter):
-    def get_filters(self, request, view):
-        filters = super(CollectionSearchFilter, self).get_filters(request, view)
-        if view.parent_resource:
-            filters.update({'owner': view.parent_resource.mnemonic})
-            filters.update({'ownerType': view.parent_resource.resource_type()})
-        else:
-            filters.update({'public_can_view': True})
-
-        return filters
+class CollectionSearchFilter(ConceptContainerPermissionedSearchFilter):
+    pass

ocl/collection/views.py

@@ -1,6 +1,8 @@
 import logging
 from django.conf import settings
+from django.contrib.contenttypes.models import ContentType
 from django.db import IntegrityError
+from django.db.models import Q
 
 from collection.validation_messages import HEAD_OF_CONCEPT_ADDED_TO_COLLECTION, CONCEPT_ADDED_TO_COLLECTION_FMT, \
     HEAD_OF_MAPPING_ADDED_TO_COLLECTION, MAPPING_ADDED_TO_COLLECTION_FMT
@@ -9,32 +11,25 @@
     CollectionVersionListSerializer, CollectionVersionCreateSerializer, CollectionVersionDetailSerializer, \
     CollectionVersionUpdateSerializer, \
     CollectionReferenceSerializer
-from concepts.models import Concept, ConceptVersion
-from mappings.models import MappingVersion
-from sources.models import SourceVersion
-from concepts.serializers import ConceptListSerializer
+from concepts.models import Concept
 from django.http import HttpResponse, HttpResponseForbidden
-from mappings.models import Mapping
-from mappings.serializers import MappingDetailSerializer
 from oclapi.mixins import ListWithHeadersMixin
 from oclapi.permissions import CanViewConceptDictionary, CanEditConceptDictionary, CanViewConceptDictionaryVersion, \
     CanEditConceptDictionaryVersion, HasOwnership
 from oclapi.permissions import HasAccessToVersionedObject
 from oclapi.views import ResourceVersionMixin, ResourceAttributeChildMixin, ConceptDictionaryUpdateMixin, \
-    ConceptDictionaryCreateMixin, ConceptDictionaryExtrasView, ConceptDictionaryExtraRetrieveUpdateDestroyView, \
-    BaseAPIView
-from oclapi.models import ACCESS_TYPE_EDIT, ACCESS_TYPE_VIEW
+    ConceptDictionaryCreateMixin, ConceptDictionaryExtrasView, ConceptDictionaryExtraRetrieveUpdateDestroyView
+from oclapi.models import ACCESS_TYPE_NONE
 from rest_framework import mixins, status
-from rest_framework.generics import RetrieveAPIView, UpdateAPIView, get_object_or_404, DestroyAPIView
+from rest_framework.generics import RetrieveAPIView, UpdateAPIView, DestroyAPIView
 from rest_framework.response import Response
 from users.models import UserProfile
 from orgs.models import Organization
 from tasks import export_collection, add_references
 from celery_once import AlreadyQueued
 from django.shortcuts import get_list_or_404
 from collection.filters import CollectionSearchFilter
-from tasks import update_collection_in_solr, delete_resources_from_collection_in_solr
-from django.core.exceptions import ValidationError
+from tasks import delete_resources_from_collection_in_solr
 
 logger = logging.getLogger('oclapi')
 
@@ -53,22 +48,19 @@ def get_version_detail_serializer(self, obj, data=None, files=None, partial=Fals
 
     def get_queryset(self):
         owner = self.get_owner()
-        if not self.kwargs:
-            return self.queryset
+        if not owner:
+            return self.queryset.exclude(public_access=ACCESS_TYPE_NONE)
         elif 'collection' in self.kwargs:
-            return Collection.objects.filter(parent_id=owner.id, mnemonic=self.kwargs['collection'])
+            return self.model.objects.filter(
+                parent_id=owner.id,
+                mnemonic=self.kwargs['collection'],
+                parent_type=ContentType.objects.get_for_model(owner),
+            )
         else:
-            return self.queryset.filter(parent_id=owner.id)
+            return self.queryset.filter(parent_id=owner.id, parent_type=ContentType.objects.get_for_model(owner))
 
     def get_owner(self):
-        owner = None
-        if 'user' in self.kwargs:
-            owner_id = self.kwargs['user']
-            owner = UserProfile.objects.get(mnemonic=owner_id)
-        elif 'org' in self.kwargs:
-            owner_id = self.kwargs['org']
-            owner = Organization.objects.get(mnemonic=owner_id)
-        return owner
+        return self.parent_resource
 
 
 class CollectionVersionBaseView(ResourceVersionMixin):
@@ -291,31 +283,14 @@ def get(self, request, *args, **kwargs):
         self.serializer_class = CollectionDetailSerializer if self.is_verbose(request) else CollectionListSerializer
         self.contains_uri = request.QUERY_PARAMS.get('contains', None)
         self.user = request.QUERY_PARAMS.get('user', None)
-        # Running the filter_backends seems to reset changes made to the queryset.
-        # Therefore, remove the filter_backends when the 'contains' parameter is passed, and
-        # apply the appropriate public_access filter in get_queryset
-        # TODO correct the behavior of filter_backends, and remove this hack to get around it
-        if self.contains_uri != None:
-            self.filter_backends=[]
-        if self.user != None:
-            self.filter_backends=[]
         collection_list = self.list(request, *args, **kwargs)
         return collection_list
 
     def get_queryset(self):
         queryset = super(CollectionListView, self).get_queryset()
-        # If the 'contains' parameter is used, the filter_backends have been cleared to prevent
-        # reset of the queryset.  Therefore, add a public_access filter to the queryset.
-        # TODO correct the behavior of filter_backends, and remove this hack to get around it
-        if self.contains_uri != None:
+        if self.contains_uri is not None:
             from django_mongodb_engine.query import A
-            queryset = queryset.filter(references=A('expression', self.contains_uri), public_access__in=[ACCESS_TYPE_EDIT, ACCESS_TYPE_VIEW])
-        if self.user:
-            if self.user != 'root':
-                from users.models import UserProfile
-                user_profile = UserProfile.objects.filter(mnemonic=self.user)
-                if user_profile:
-                    queryset = queryset.filter(parent_id__in=[user_profile[0].id] + user_profile[0].organizations, public_access__in=[ACCESS_TYPE_EDIT, ACCESS_TYPE_VIEW])
+            queryset = queryset.filter(references=A('expression', self.contains_uri))
         return queryset
 
     def get_csv_rows(self, queryset=None):