diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index d3dc2346b..94df5b92b 100644 --- a/dataservice/api/common/model.py +++ b/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 diff --git a/dataservice/api/common/schemas.py b/dataservice/api/common/schemas.py index 54e2ca923..0d1c51a36 100644 --- a/dataservice/api/common/schemas.py +++ b/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') diff --git a/dataservice/extensions/flask_indexd.py b/dataservice/extensions/flask_indexd.py index 37fcebc62..2bbbb4d73 100644 --- a/dataservice/extensions/flask_indexd.py +++ b/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,16 +224,13 @@ 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' @@ -275,8 +238,6 @@ def delete(self, record): 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 diff --git a/tests/genomic_file/test_genomic_file_models.py b/tests/genomic_file/test_genomic_file_models.py index 1f3106811..08bccce00 100644 --- a/tests/genomic_file/test_genomic_file_models.py +++ b/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': {} } diff --git a/tests/genomic_file/test_genomic_file_resources.py b/tests/genomic_file/test_genomic_file_resources.py index 8af62e840..142cf97c8 100644 --- a/tests/genomic_file/test_genomic_file_resources.py +++ b/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)) diff --git a/tests/mocks.py b/tests/mocks.py index 54f950e98..8ca8bccdd 100644 --- a/tests/mocks.py +++ b/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 diff --git a/tests/study_file/test_study_file_models.py b/tests/study_file/test_study_file_models.py index 260bd0af7..1470e6bbe 100644 --- a/tests/study_file/test_study_file_models.py +++ b/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'}