Merge pull request #548 from kids-first/revert-539-indexd-authz

⏪ Revert authz changes

dataservice/api/common/model.py

@@ -82,7 +82,6 @@ class IndexdFile:
     rev = None
     hashes = {}
     acl = []
-    authz = []
     # The metadata property is already used by sqlalchemy
     _metadata = {}
     size = None
@@ -100,7 +99,6 @@ def constructor(self):
         self.rev = None
         self.hashes = {}
         self.acl = []
-        self.authz = []
         # The metadata property is already used by sqlalchemy
         self._metadata = {}
         self.size = None

dataservice/api/common/schemas.py

@@ -111,20 +111,10 @@ def check_unknown_fields(self, data, original_data):
             raise ValidationError('Unknown field', unknown)
 
 
-def acl_deprecation(v):
-    if v is not None and not v == []:
-        raise ValidationError(
-            "The ACL field has been disabled. Use the authz field instead."
-        )
-
-
 class IndexdFileSchema(Schema):
     urls = ma.List(ma.Str(), required=True)
     access_urls = ma.List(ma.Str(), dump_only=True)
-    acl = ma.List(
-        ma.Str(), required=False, validate=lambda v: acl_deprecation(v)
-    )
-    authz = ma.List(ma.Str(), required=False)
+    acl = ma.List(ma.Str(), required=False)
     file_name = ma.Str()
     hashes = ma.Dict(required=True)
     metadata = ma.Dict(attribute='_metadata')

dataservice/extensions/flask_indexd.py

@@ -3,7 +3,7 @@
 
 from flask import current_app, abort
 from flask import _app_ctx_stack as stack
-from requests.exceptions import HTTPError, RequestException
+from requests.exceptions import HTTPError
 
 
 class RecordNotFound(HTTPError):
@@ -55,14 +55,9 @@ def get(self, record):
             return record
 
         url = self.url + record.latest_did
-        try:
-            resp = self.session.get(url)
-            self.check_response(resp)
-            resp.raise_for_status()
-        except RecordNotFound as err:
-            raise err
-        except RequestException as err:
-            abort(500, f"Problem getting record from Indexd: {err}")
+        resp = self.session.get(url)
+        self.check_response(resp)
+        resp.raise_for_status()
 
         # update fields on the target record's object
         for prop, v in resp.json().items():
@@ -82,8 +77,7 @@ def new(self, record):
 
           {
             "file_name": "my_file",
-            "acl": [],
-            "authz": ["/programs/phs000000"],
+            "acl": ["phs000000"],
             "hashes": {
               "md5": "0b7940593044dff8e74380476b2b27a9"
             },
@@ -122,18 +116,15 @@ def new(self, record):
             "form": "object",
             "hashes": record.hashes,
             "acl": record.acl,
-            "authz": record.authz,
             "urls": record.urls,
             "metadata": meta
         }
 
         # Register the file on indexd
-        try:
-            resp = self.session.post(self.url, json=req_body)
-            resp.raise_for_status()
-        except RequestException as err:
-            abort(500, f"Problem creating new file in Indexd: {err}")
+        resp = self.session.post(self.url,
+                                 json=req_body)
 
+        resp.raise_for_status()
         resp = resp.json()
 
         # Update the record object with the id fields
@@ -155,11 +146,7 @@ def update(self, record):
 
         # Fetch rev for the did
         url = self.url + record.latest_did
-        try:
-            resp = self.session.get(url)
-            resp.raise_for_status()
-        except RequestException as err:
-            abort(500, f"Problem getting record from Indexd: {err}")
+        resp = self.session.get(url)
         self.check_response(resp)
 
         old = resp.json()
@@ -170,7 +157,6 @@ def update(self, record):
             "size": record.size,
             "hashes": record.hashes,
             "acl": record.acl,
-            "authz": record.authz,
             "urls": record.urls,
             "metadata": record._metadata
         }
@@ -180,30 +166,20 @@ def update(self, record):
             del req_body['size']
             del req_body['hashes']
 
-        # Update all previous versions with the new authz or acl fields, if
-        # they changed
-        if record.authz != old.get('authz') or record.acl != old.get('acl'):
+        # If acl changed, update all previous version with new acl
+        if record.acl != old['acl']:
             self._update_all_acls(record)
 
         url = '{}{}?rev={}'.format(self.url, record.latest_did, record.rev)
         if 'size' in req_body or 'hashes' in req_body:
             # Create a new version in indxed
             req_body['form'] = 'object'
-            try:
-                resp = self.session.post(url, json=req_body)
-                resp.raise_for_status()
-            except RequestException as err:
-                abort(500, f"Problem creating a new record in Indexd: {err}")
-
-            did = resp.json()["did"]
+            resp = self.session.post(url, json=req_body)
+            did = resp.json()['did']
             record.latest_did = did
         else:
             # Update the file on indexd
-            try:
-                resp = self.session.put(url, json=req_body)
-                resp.raise_for_status()
-            except RequestException as err:
-                abort(500, f"Problem updating record in Indexd: {err}")
+            resp = self.session.put(url, json=req_body)
 
         self.check_response(resp)
         resp.raise_for_status()
@@ -212,36 +188,26 @@ def update(self, record):
 
     def _update_all_acls(self, record):
         """
-        Update acls and authz for all previous versions of a record and
-        update the target record's rev
+        Update acls for all previous versions of a record and update the
+        target record's rev
         """
         # Only use fields allowed by the indexd PUT schema
-        fields = ['urls', 'acl', 'authz', 'file_name', 'version',
+        fields = ['urls', 'acl', 'file_name', 'version',
                   'metadata', 'urls_metadata', 'rev']
 
         url = '{}{}/versions'.format(self.url, record.latest_did)
-        try:
-            resp = self.session.get(url)
-            resp.raise_for_status()
-            versions = resp.json()
-        except RequestException as err:
-            abort(500, f"Problem updating record in Indexd: {err}")
+        versions = self.session.get(url).json()
         for version, doc in versions.items():
-            if doc['acl'] != record.acl or doc['authz'] != record.authz:
+            if doc['acl'] != record.acl:
                 did = doc['did']
                 doc = {k: v for k, v in doc.items() if k in fields}
                 doc['acl'] = record.acl
-                doc['authz'] = record.authz
                 if doc['version'] is None:
                     del doc['version']
                 url = '{}{}?rev={}'.format(self.url, did, doc['rev'])
                 # rev is not allowed in put schema
                 del doc['rev']
-                try:
-                    resp = self.session.put(url, json=doc)
-                    resp.raise_for_status()
-                except RequestException as err:
-                    abort(500, f"Problem updating record in Indexd: {err}")
+                resp = self.session.put(url, json=doc)
                 # Update the record's rev if it's the record being modified
                 if record.latest_did == did:
                     record.rev = resp.json()['rev']
@@ -258,25 +224,20 @@ def delete(self, record):
             return record
 
         if record.rev is None:
-            try:
-                r = self.session.get(self.url + record.latest_did)
-            except RequestException as err:
-                abort(500, f"Problem getting record in Indexd: {err}")
+            r = self.session.get(self.url + record.latest_did)
             record.rev = r.json()['rev']
 
         url = '{}{}?rev={}'.format(self.url, record.latest_did, record.rev)
+        resp = self.session.delete(url)
+        self.check_response(resp)
         try:
-            resp = self.session.delete(url)
-            self.check_response(resp)
             resp.raise_for_status()
         except HTTPError:
             message = 'could not get file record'
             # Attach indexd error message, if there is one
             if 'error' in resp.json():
                 message = '{}: {}'.format(message, resp.json()['error'])
             abort(resp.status_code, message)
-        except RequestException as err:
-            abort(500, f"Problem deleting record in Indexd: {err}")
 
         return record
 

tests/genomic_file/test_genomic_file_models.py

@@ -1,7 +1,6 @@
 import datetime
 import uuid
 import random
-from werkzeug.exceptions import BadRequest
 
 from dataservice.extensions import db
 from dataservice.api.study.models import Study
@@ -110,7 +109,7 @@ def test_update_indexd_only(self):
         gf = GenomicFile.query.get(kwargs['kf_id'])
         gf.external_id = 'blah'
         gf.size = 1234
-        gf.authz = ['/programs/new_acl']
+        gf.acl = ['new_acl']
         gf._metadata = {'test': 'test'}
         did = gf.latest_did
         db.session.commit()
@@ -124,8 +123,7 @@ def test_update_indexd_only(self):
         expected = MockIndexd.doc_base.copy()
         expected.update({
             'size': 1234,
-            'acl': [],
-            'authz': ["/programs/new_acl"],
+            'acl': ["new_acl"],
             'metadata': {'test': 'test'},
         })
         self.indexd.Session().post.assert_any_call(
@@ -158,42 +156,6 @@ def test_update_acl_only(self):
         expected = {
             'file_name': 'hg38.bam',
             'acl': ['INTERNAL', 'new_acl'],
-            'authz': ['/programs/INTERNAL'],
-            'urls': ['s3://bucket/key'],
-            'metadata': {}
-        }
-        self.indexd.Session().put.assert_any_call(
-            '{}?rev={}'.format(did, gf.rev), json=expected)
-
-
-    def test_update_authz_only(self):
-        """
-        Test updating of only authz field
-        """
-        # Create and save genomic files and dependent entities
-        kwargs_dict = self._create_save_genomic_files()
-
-        kwargs = kwargs_dict[list(kwargs_dict.keys())[1]]
-
-        # Update fields
-        gf = GenomicFile.query.get(kwargs['kf_id'])
-        gf.authz = ['/programs/INTERNAL', '/programs/new_authz']
-        did = gf.latest_did
-        # explicitly tell the object to update one of the mapped fields
-        gf.modified_at = datetime.datetime.now()
-        db.session.commit()
-
-        # Check database
-        gf = GenomicFile.query.get(kwargs['kf_id'])
-        assert gf.authz == ['/programs/INTERNAL', '/programs/new_authz']
-
-        # Update document and all versions
-        assert self.indexd.Session().put.call_count == 4
-
-        expected = {
-            'file_name': 'hg38.bam',
-            'acl': [],
-            'authz': ['/programs/INTERNAL', '/programs/new_authz'],
             'urls': ['s3://bucket/key'],
             'metadata': {}
         }

tests/genomic_file/test_genomic_file_resources.py

@@ -103,7 +103,6 @@ def test_new_indexd_error(client, entities):
         'file_name': 'hg38.bam',
         'size': 123,
         'acl': ['TEST'],
-        'authz': ['/projects/TEST'],
         'data_type': 'Aligned Reads',
         'file_format': 'bam',
         'urls': ['s3://bucket/key'],
@@ -121,35 +120,6 @@ def test_new_indexd_error(client, entities):
     assert GenomicFile.query.count() == init_count
 
 
-def test_no_acl(client, entities, genomic_files):
-    """
-    Test that acls may no longer be specified during creation
-    """
-
-    body = {
-        'external_id': 'genomic_file_0',
-        'file_name': 'hg38.bam',
-        'size': 123,
-        'acl': ['TEST'],
-        'authz': ['/projects/TEST'],
-        'data_type': 'Aligned Reads',
-        'file_format': 'bam',
-        'urls': ['s3://bucket/key'],
-        'controlled_access': False,
-        'hashes': {'md5': 'd418219b883fce3a085b1b7f38b01e37'}
-    }
-    init_count = GenomicFile.query.count()
-    response = client.post(url_for(GENOMICFILE_LIST_URL),
-                           headers={'Content-Type': 'application/json'},
-                           data=json.dumps(body))
-
-    resp = json.loads(response.data.decode("utf-8"))
-
-    assert 400 == response.status_code
-    assert 'could not create' in resp['_status']['message']
-    assert GenomicFile.query.count() == init_count
-
-
 def test_get_list(client, indexd, genomic_files):
     """
     Test that genomic files are returned in a paginated list with all
@@ -213,7 +183,6 @@ def test_get_one(client, entities):
     assert 'rev' not in resp
     assert resp['size'] == gf.size
     assert resp['acl'] == gf.acl
-    assert resp['authz'] == gf.authz
     # check properties from datamodel
     assert resp['file_name'] == gf.file_name
     assert resp['data_type'] == gf.data_type
@@ -231,8 +200,6 @@ def test_update_no_version(client, indexd):
 
     genomic_file = resp['results']
     body = genomic_file
-    body['authz'] = ['/programs/new-auth']
-    del body['acl']
     response = client.patch(url_for(GENOMICFILE_LIST_URL) + '/' + body['kf_id'],
                             headers={'Content-Type': 'application/json'},
                             data=json.dumps(body))

tests/mocks.py

@@ -51,8 +51,7 @@ class MockIndexd(MagicMock):
         "did": "",
         "metadata": {
         },
-        "acl": [],
-        "authz": ["/programs/INTERNAL"],
+        "acl": ["INTERNAL"],
         "rev": "39b19b2d",
         "updated_date": "2018-02-21T00:44:27.414671",
         "version": None

tests/study_file/test_study_file_models.py

@@ -60,8 +60,7 @@ def test_update(self):
 
     def test_update_all_versions(self):
         """
-        Test that all verions of a document are updated when the autz is
-        changed
+        Test that all verions of a document are updated when the acl is changed
         """
         # Create study_files and study
         study_files, study, data = self.create_study_file()
@@ -73,8 +72,7 @@ def test_update_all_versions(self):
         sf.size = 7696048
         sf.hashes = {'md5': 'dcff06ebb19bc9aa8f1aae1288d10dc2'}
         # Update to a new acl
-        sf.acl = []
-        sf.authz = ['/programs/new-authz']
+        sf.acl = ['new_acl']
         sf.modified_at = datetime.datetime.now()
         did = sf.latest_did
         db.session.add(sf)
@@ -99,7 +97,7 @@ def test_new_version(self):
 
         sf = StudyFile.query.get(study_files[0].kf_id)
         # Set to value returned in the mock endpoint
-        sf.authz = ['/programs/INTERNAL']
+        sf.acl = ['INTERNAL']
         # New content values
         sf.size = 1234
         sf.hashes = {'md5': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'}