Hotfix storage API security with auth checks and policy updates

modules/invenio-files-rest/invenio_files_rest/storage/pyfs.py

@@ -18,6 +18,7 @@
 from flask import current_app
 from fs.opener import opener
 from fs.path import basename, dirname
+from sqlalchemy import String, and_, func, literal, or_
 
 from ..helpers import make_path
 from .base import FileStorage, StorageError
@@ -205,7 +206,6 @@ def pyfs_storage_factory(fileinstance=None, default_location=None,
     from ..models import Location
     assert fileinstance or (fileurl and size)
     location = None
-    locationList = Location.all()
 
     if fileinstance:
         # FIXME: Code here should be refactored since it assumes a lot on the
@@ -228,13 +228,37 @@ def pyfs_storage_factory(fileinstance=None, default_location=None,
                 current_app.config['FILES_REST_STORAGE_PATH_SPLIT_LENGTH'],
             )
 
-        location = next((loc for loc in locationList if str(loc.uri) == str(default_location)), None)
+        if default_location:
+            location = Location.query.filter(Location.uri == str(default_location)).first()
 
     if location is None:
-        location = next((loc for loc in locationList if str(loc.uri) in str(fileurl)), None)
-        if location is None:
-            # if not match fileurl with location, then get default location
-            location = next((loc for loc in locationList if loc.default == True), None)
+        # Match ``Location.uri`` as a path prefix of ``fileurl``, not as a
+        # plain text prefix: a boundary is required right after the URI so
+        # that e.g. the location ``s3://bucket-a`` never matches a file
+        # stored in ``s3://bucket-a2``. Selecting the wrong location would
+        # hand out the wrong (S3) credentials for the file.
+        fileurl_expr = literal(str(fileurl), String)
+        uri_length = func.length(Location.uri)
+        location = Location.query.filter(
+            and_(
+                func.substr(fileurl_expr, 1, uri_length) == Location.uri,
+                or_(
+                    # fileurl is exactly the location URI
+                    func.length(fileurl_expr) == uri_length,
+                    # the location URI already ends with a separator
+                    func.substr(Location.uri, uri_length, 1) == '/',
+                    # the character right after the URI is a separator
+                    func.substr(fileurl_expr, uri_length + 1, 1) == '/',
+                ),
+            )
+        ).order_by(uri_length.desc()).first()
+
+    if location is None:
+        # if not match fileurl with location, then get default location
+        location = Location.query.filter_by(default=True).first()
+
+    if location is None:
+        current_app.logger.warning('No location matched. fileurl={}'.format(fileurl))
 
     return filestorage_class(
         fileurl, size=size, modified=modified, clean_dir=clean_dir, location=location)

modules/invenio-files-rest/tests/test_storage.py

@@ -17,13 +17,16 @@
 
 import pytest
 from fs.errors import DirectoryNotEmptyError, ResourceNotFoundError
-from mock import patch
+from unittest.mock import patch
 from six import BytesIO
+from sqlalchemy import event
 
 from invenio_files_rest.errors import FileSizeError, StorageError, \
     UnexpectedFileSizeError
 from invenio_files_rest.limiters import FileSizeLimit
-from invenio_files_rest.storage import FileStorage, PyFSFileStorage
+from invenio_files_rest.models import Location
+from invenio_files_rest.storage import FileStorage, PyFSFileStorage, \
+    pyfs_storage_factory
 
 
 def test_storage_interface():
@@ -348,3 +351,273 @@ def test_non_unicode_filename(app, pyfs):
             'żółć.txt', mimetype='text/plain', checksum=checksum)
         assert res.status_code == 200
         assert res.headers['Content-Disposition'] == 'inline'
+
+
+def _add_location(db, name, uri, default=False):
+    """Add a location row and commit it.
+
+    ``Location.name`` is validated against ``^[a-z][a-z0-9-]+$``
+    (``invenio_files_rest/models.py``), so names must be two characters or
+    longer, start with a lower-case letter and contain only lower-case
+    alphanumerics and dashes.
+    """
+    loc = Location(name=name, uri=uri, default=default)
+    db.session.add(loc)
+    db.session.commit()
+    return loc
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_prefix_match -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_prefix_match(app, db, dummy_location):
+    """Test that a location whose URI prefixes the fileurl is selected."""
+    _add_location(db, 'loc-a', 's3://bucket-a')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a/ab/cd/ef/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name == 'loc-a'
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_longest_prefix_wins -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_longest_prefix_wins(app, db, dummy_location):
+    """Test that the longest matching location URI wins.
+
+    The shorter URI is inserted first on purpose: without the
+    ``ORDER BY length(uri) DESC`` clause PostgreSQL returns rows in physical
+    (insert) order, so dropping the ordering makes this test fail.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+    _add_location(db, 'loc-b', 's3://bucket-a/sub')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a/sub/ab/cd/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name == 'loc-b'
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_no_partial_match -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_no_partial_match(app, db, dummy_location):
+    """Test that a location URI matches only at the start of the fileurl.
+
+    ``/mnt/other`` appears in the fileurl but not as a prefix, so it must not
+    be selected and the default location must be used instead.
+    """
+    _add_location(db, 'loc-x', '/mnt/other')
+
+    storage = pyfs_storage_factory(fileurl='/mnt/data/backup/mnt/other/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name != 'loc-x'
+    assert storage.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_uri_underscore_not_wildcard -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_uri_underscore_not_wildcard(
+        app, db, dummy_location):
+    """Test that an underscore in a location URI is not a LIKE wildcard."""
+    _add_location(db, 'loc-us', 's3://weko_bucket')
+
+    storage = pyfs_storage_factory(fileurl='s3://wekoxbucket/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name != 'loc-us'
+    assert storage.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_default_fallback -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_default_fallback(app, db, dummy_location):
+    """Test the fallback to the default location when nothing matches."""
+    storage = pyfs_storage_factory(fileurl='s3://nowhere/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_no_location_logs_warning -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_no_location_logs_warning(app, db, mocker):
+    """Test that a warning is logged when no location can be resolved.
+
+    No location fixture is requested on purpose: with a default location
+    present the fallback would succeed and no warning would be emitted.
+    """
+    warning_mock = mocker.patch.object(app.logger, 'warning')
+
+    storage = pyfs_storage_factory(fileurl='s3://nowhere/ab/data', size=1)
+
+    assert storage.location is None
+    warning_mock.assert_called_once()
+    assert 's3://nowhere/ab/data' in warning_mock.call_args[0][0]
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_default_location_match -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_default_location_match(
+        app, db, dummy_location, mocker):
+    """Test that an explicit default_location takes precedence.
+
+    ``loc-a`` prefixes the fileurl and would win the prefix lookup, so it also
+    proves that the prefix lookup is not executed once the URI of
+    ``default_location`` has been resolved.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+
+    fileinstance = mocker.MagicMock()
+    fileinstance.size = 1
+    fileinstance.updated = None
+    fileinstance.uri = 's3://bucket-a/ab/data'
+
+    storage = pyfs_storage_factory(
+        fileinstance=fileinstance, default_location=dummy_location.uri)
+
+    assert storage.location is not None
+    assert storage.location.name != 'loc-a'
+    assert storage.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_skips_query_when_no_default_location -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_skips_query_when_no_default_location(
+        app, db, mocker):
+    """Test that no query is issued when default_location is not given.
+
+    ``loc-none`` has the literal URI ``'None'``: without the guard the lookup
+    would compare against ``str(None)`` and select it.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+    _add_location(db, 'loc-none', 'None')
+
+    fileinstance = mocker.MagicMock()
+    fileinstance.size = 1
+    fileinstance.updated = None
+    fileinstance.uri = 's3://bucket-a/ab/data'
+
+    statements = []
+
+    def _record(conn, cursor, statement, parameters, context, executemany):
+        statements.append(statement)
+
+    event.listen(db.engine, 'before_cursor_execute', _record)
+    try:
+        storage = pyfs_storage_factory(fileinstance=fileinstance)
+    finally:
+        event.remove(db.engine, 'before_cursor_execute', _record)
+
+    assert storage.location is not None
+    assert storage.location.name != 'loc-none'
+    assert storage.location.name == 'loc-a'
+    assert len(statements) == 1
+    assert 'substr' in statements[0].lower()
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_no_full_scan -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_no_full_scan(app, db, dummy_location, mocker):
+    """Test that the whole location table is never loaded into memory."""
+    _add_location(db, 'loc-a', 's3://bucket-a')
+    mock_all = mocker.patch('invenio_files_rest.models.Location.all')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a/ab/data', size=1)
+
+    mock_all.assert_not_called()
+    assert storage.location is not None
+    assert storage.location.name == 'loc-a'
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_passes_args_to_filestorage_class -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_passes_args_to_filestorage_class(app, db, dummy_location, mocker):
+    """Test the arguments handed over to the file storage class."""
+    loc_a = _add_location(db, 'loc-a', 's3://bucket-a')
+    fake_class = mocker.MagicMock()
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a/ab/data', size=1, filestorage_class=fake_class)
+
+    fake_class.assert_called_once_with('s3://bucket-a/ab/data', size=1, modified=None, clean_dir=True, location=loc_a)
+    assert fake_class.call_args[1]['location'].name == 'loc-a'
+    assert storage is fake_class.return_value
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_similar_bucket_name_not_matched -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_similar_bucket_name_not_matched(
+        app, db, dummy_location):
+    """Test that a location URI only matches on a path boundary.
+
+    ``s3://bucket-a`` is a plain text prefix of ``s3://bucket-a2/...`` but not
+    a path prefix of it. Without the boundary condition ``loc-a`` would be
+    selected and would supply the S3 credentials of the wrong account for a
+    file that actually lives in another bucket.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a2/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name != 'loc-a'
+    assert storage.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_uri_with_trailing_slash -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_uri_with_trailing_slash(app, db, dummy_location):
+    """Test that a location URI already ending with ``/`` still matches.
+
+    The boundary must not be required twice: for ``s3://bucket-b/`` the
+    separator is part of the URI itself, so the character following it is a
+    regular path character and the location must still be selected.
+    """
+    _add_location(db, 'loc-b', 's3://bucket-b/')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-b/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name == 'loc-b'
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_similar_bucket_names_coexist -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_similar_bucket_names_coexist(
+        app, db, dummy_location):
+    """Test that similarly named buckets each resolve to their own location.
+
+    Both ``s3://bucket-a`` and ``s3://bucket-a2`` are registered, so a purely
+    textual prefix match would resolve both file URLs to ``loc-a`` and mix up
+    the credentials of the two buckets.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+    _add_location(db, 'loc-a2', 's3://bucket-a2')
+
+    storage_a = pyfs_storage_factory(fileurl='s3://bucket-a/ab/data', size=1)
+    storage_a2 = pyfs_storage_factory(fileurl='s3://bucket-a2/ab/data', size=1)
+
+    assert storage_a.location is not None
+    assert storage_a.location.name == 'loc-a'
+    assert storage_a2.location is not None
+    assert storage_a2.location.name == 'loc-a2'
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_local_path_boundary -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_local_path_boundary(app, db, dummy_location):
+    """Test that the boundary also applies to local file system locations.
+
+    ``/mnt/data`` must not swallow files stored below ``/mnt/data2``, which
+    may be a completely different mount point.
+    """
+    _add_location(db, 'loc-data', '/mnt/data')
+    _add_location(db, 'loc-data2', '/mnt/data2')
+
+    storage = pyfs_storage_factory(fileurl='/mnt/data2/ab/data', size=1)
+    storage_other = pyfs_storage_factory(fileurl='/mnt/database/ab/data', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name == 'loc-data2'
+    assert storage_other.location is not None
+    assert storage_other.location.id == dummy_location.id
+
+
+# .tox/c1/bin/pytest --cov=invenio_files_rest tests/test_storage.py::test_pyfs_storage_factory_exact_uri_match -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/invenio-files-rest/.tox/c1/tmp
+def test_pyfs_storage_factory_exact_uri_match(app, db, dummy_location):
+    """Test that a fileurl equal to the location URI still matches.
+
+    There is no character left after the URI to carry the separator, so the
+    boundary check has to accept an exact match as well.
+    """
+    _add_location(db, 'loc-a', 's3://bucket-a')
+
+    storage = pyfs_storage_factory(fileurl='s3://bucket-a', size=1)
+
+    assert storage.location is not None
+    assert storage.location.name == 'loc-a'

modules/weko-records-ui/tests/conftest.py

@@ -79,7 +79,7 @@
 from invenio_search_ui import InvenioSearchUI
 from invenio_theme import InvenioTheme
 from six import BytesIO
-from sqlalchemy_utils.functions import create_database, database_exists
+from sqlalchemy_utils.functions import create_database, database_exists, drop_database
 from weko_admin import WekoAdmin
 from weko_admin.models import SessionLifetime
 from weko_admin.models import AdminSettings
@@ -380,8 +380,9 @@ def esindex(app):
 @pytest.yield_fixture()
 def db(app):
     """Database fixture."""
-    if not database_exists(str(db_.engine.url)):
-        create_database(str(db_.engine.url))
+    if database_exists(str(db_.engine.url)):
+        drop_database(str(db_.engine.url))
+    create_database(str(db_.engine.url))
     db_.create_all()
     yield db_
     db_.session.remove()