Feature: authenticated file upload #463

1yam · 2023-08-21T14:21:58Z

Problem: file upload is performed in two steps, one to push the file and one to push the associated STORE message.

Solution: improve /api/v0/storage/add_file to allow the user to send a STORE message along with his file, making upload an atomic operation.

odesenfans

I still need to review the tests. There are a lot of things to change in the implementation as it is.

odesenfans · 2023-08-23T08:21:26Z

src/aleph/web/controllers/storage.py

+from aleph.web.controllers.p2p import (
+    _mq_read_one_message,
+    _processing_status_to_http_status,
+)


In Python, functions starting with an underscore should not be imported. What you need to do is move these functions to a common module (ex: controllers/utils.py) and rename them to remove the underscore (=making them public).

odesenfans · 2023-08-23T08:27:16Z

src/aleph/web/controllers/storage.py

+
+    except Exception as e:
+        verified = False
+    return verified


This is very clearly duplicated from the code in ChainService. The right thing to do here is to add an instance of ChainService to each API in api_entrypoint.py:

Create an APP_CHAIN_SERVICE constant and a corresponding getter in app_state_getters.py

Instantiate a chain service in api_entrypoint.py

Add it to the app state (app[APP_CHAIN_SERVICE] = chain_service)

Use the getter in this file to verify the signature.

chain_service = get_chain_service_from_request(request) await chain_service.verify_signature(message)

At the moment it is half implemented (must be added to ccn_api_client to be able to use it)

odesenfans · 2023-08-23T08:28:09Z

src/aleph/web/controllers/storage.py

+    elif actual_item_hash != c_item_hash:
+        output = {"status": "Unprocessable Content"}
+        return web.json_response(output, status=422)
+    elif len(content) > 25_000 and not message:


Define constants like this 25_000 at the top of the file. Also, out of consistency now that we decided to use MiB everywhere, define it as 25 * MiB.

odesenfans · 2023-08-23T08:31:34Z

src/aleph/web/controllers/storage.py

+    post_data: MultiDictProxy[Union[str, bytes, FileField]]
+) -> Tuple[dict, int]:
+    message_bytearray = post_data.get("message", b"")
+    value = post_data.get("size") or 0


Okay so I actually expected you to put the message and metadata in the body of the request and not in the multipart form. Why did you put it in the form?

Also, use better variable names than value. You're retrieving a size here, so size or file_size are definitely clearer.

Also, we probably need a maximum size somewhere. Put 1 GiB at the moment and return a 413 (Payload too large) if the file is larger.

odesenfans · 2023-08-23T08:36:16Z

src/aleph/web/controllers/storage.py

+    if isinstance(message_bytearray, bytearray):
+        message_string = message_bytearray.decode("utf-8")
+        message_dict = json.loads(message_string)
+        message_dict["time"] = float(message_dict["time"])


A lot of validation is missing. You need to use the BasePendingMessage to parse the message dictionary, catch errors and raise a 422 if the message is not correct. See how it's done in POST /messages (_validate_message_dict). You can directly use PendingStoreMessage.parse_obj(message_dict) as we only want to process STORE messages and ignore the rest.

odesenfans · 2023-08-23T09:03:55Z

src/aleph/web/controllers/storage.py

+    content = file_io.read(size)
+    item_content = json.loads(message["item_content"])
+    actual_item_hash = sha256(content).hexdigest()
+    c_item_hash = item_content["item_hash"]


I don't get why you prefixed the variable with c_ ?

c == client on my head.
It's the hash give by the "client"

odesenfans · 2023-08-23T09:11:07Z

src/aleph/web/controllers/storage.py

+        routing_key=f"*.{pending_message_db.item_hash}",
+    )
+
+    is_valid_message = await verify_and_handle_request(


Poor variable name. is_valid_name implies a boolean value (True if the message is valid, false otherwise). To improve this, you can either define your custom exception types or use aiohttp exceptions in verify_and_handle_request.

Example:

# In verify_and_handle_request if balance < required_balance: raise web.HTTPPaymentRequired() # Here try: verify_and_handle_request(...) except web.HTTPClientError as e: return web.json_response({"status": "rejected"}, status=e.status_code)

src/aleph/web/controllers/storage.py

odesenfans · 2023-08-23T09:26:01Z

src/aleph/web/controllers/storage.py

+        message_dict=message, reception_time=dt.datetime.now(), fetched=True
+    )
+    mq_channel = await get_mq_channel_from_request(request, logger=logger)
+    mq_queue = await mq_make_aleph_message_topic_queue(


The MQ stuff only needs to be done if a) there is a message attached to the request and b) the user specified sync=True in the request. The sync parameter is nowhere to be found in your PR, add it.

odesenfans · 2023-08-23T09:40:11Z

src/aleph/web/controllers/storage.py

+async def storage_add_file(request: web.Request):
+    post = await request.post()
+    if post.get("message", b"") is not None and post.get("size") is not None:
+        return await storage_add_file_with_message(request)


This split is not ideal because you duplicate the part where you read the file and add it in the DB. Also, there is a major bug in your new function because you never actually write the file to disk nor add the corresponding entry in the files table. I'd rather have one function where you call other functions, example (fleshed out, just to give an example):

if message and size: pending_message = await verify_message(message) await verify_balance(pending_message.content.address, size) expected_file_hash = pending_message.content.item_hash else: expected_file_hash = None file_io = multidict_proxy_to_io(post) file_content, item_hash = await read_and_check_file(file_io, expected_file_hash) # Note that this requires overloading the `StorageService.add_file` method to support passing bytes instead of an IO object await storage_service.add_file(session=session, file=file_content, engine=ItemType.storage)

odesenfans · 2023-08-25T12:47:13Z

src/aleph/web/controllers/p2p.py

@@ -26,7 +26,8 @@
    get_p2p_client_from_request,
    get_mq_channel_from_request,
 )
-from aleph.web.controllers.utils import mq_make_aleph_message_topic_queue
+from aleph.web.controllers.utils import mq_make_aleph_message_topic_queue, processing_status_to_http_status, \
+    mq_read_one_message, validate_message_dict


Run the black formatter on your PR.

odesenfans · 2023-08-28T08:23:57Z

src/aleph/api_entrypoint.py

@@ -67,6 +74,7 @@ async def configure_aiohttp_app(
        app[APP_STATE_NODE_CACHE] = node_cache
        app[APP_STATE_STORAGE_SERVICE] = storage_service
        app[APP_STATE_SESSION_FACTORY] = session_factory
+        # app[APP_STATE_CHAIN_SERVICE] = chain_service


Why is this line commented?

src/aleph/web/controllers/storage.py

odesenfans · 2023-08-28T08:34:22Z

src/aleph/web/controllers/storage.py

+
+
+async def _verify_user_file(message: PendingStoreMessage, size: int, file_io) -> None:
+    file_io.seek(0)


Why do you need to seek? Are you reading the file twice?

file_content = fileobject.read()

The functions add_file on Storage Service class wasn't doing seek when after read (he was doing it for ipf, I replace the seek here and not on _verify_user_file

We really shouldn't read the file twice, that's an expensive operation.

odesenfans · 2023-08-28T08:35:35Z

src/aleph/web/controllers/storage.py

+
+    session.add(pending_message_db)
+    session.commit()
+    if storage_metadata.sync and mq_queue:


Suggested change

if storage_metadata.sync and mq_queue:

if mq_queue:

is enough.

odesenfans · 2023-08-28T08:36:58Z

src/aleph/web/controllers/storage.py

+
+        if mq_message is None:
+            raise web.HTTPAccepted()
+        if mq_message.routing_key is not None:


What happens if routing_key is None?

Note that since you subscribe using a routing key, this case should never happen.

odesenfans · 2023-08-28T08:37:20Z

src/aleph/web/controllers/storage.py

-
-    # No need to pin it here anymore.
-    # TODO: find a way to specify linked ipfs hashes in posts/aggr.
+    # TODO : Add chainservice to ccn_api_client to be able to call get_chainservice_from_request


odesenfans

The logic of the endpoint is still not what I'd expect. You read the file multiple times, add it to the storage before checks, etc.

odesenfans · 2023-09-04T09:29:39Z

src/aleph/storage.py

@@ -271,7 +273,7 @@ async def add_file(
        elif engine == ItemType.storage:
            file_content = fileobject.read()
            file_hash = sha256(file_content).hexdigest()
-
+            fileobject.seek(0)


As previously discussed, we should never have to read a file twice. If the signature of the function doesn't allow what you need, change it/add a new function to support your use case.

odesenfans · 2023-09-04T09:38:56Z

src/aleph/web/controllers/storage.py

+    current_balance = get_total_balance(
+        session=session, address=pending_message_db.sender
+    ) or Decimal(0)
+    required_balance = (size / MiB) / 3


I think the computation in the view is incorrect, as it still uses MB instead of MiB and counts 3 Aleph/MB instead of 3 MiB/Aleph. Could you fix this (in a different PR if possible)?

https://github.com/1yam/pyaleph/blob/eb91e2190ef4cd7c941a173b5a68705a0b42e31f/deployment/migrations/versions/0018_7bcb8e5fe186_fix_vm_cost_view.py#L96

1048576 = (1024 * 1024) so i thinks the view is correct ?

odesenfans · 2023-09-04T09:41:08Z

src/aleph/web/controllers/storage.py

+        status_str, _item_hash = mq_message.routing_key.split(".")
+        processing_status = MessageProcessingStatus(status_str)
+        status_code = processing_status_to_http_status(processing_status)
+        return web.json_response(status=status_code, text=_item_hash)


You're not returning a response anymore in the case if not mq_queue.

https://github.com/1yam/pyaleph/blob/e625014f76ced48de20fc3279b5e5ac23ce30634/src/aleph/web/controllers/storage.py#L218

i am

odesenfans · 2023-09-04T09:42:30Z

src/aleph/web/controllers/storage.py

+
+
+async def storage_add_file_with_message(
+    request: web.Request, session: DbSession, chain_service, storage_metadata, file_io


Missing some typing:

Suggested change

request: web.Request, session: DbSession, chain_service, storage_metadata, file_io

request: web.Request, session: DbSession, chain_service: ChainService, storage_metadata: StorageMetadata, file_io: BinaryIO

odesenfans · 2023-09-04T09:43:44Z

src/aleph/web/controllers/storage.py

+    config = get_config_from_request(request)
+    mq_queue = None
+
+    pending_store_message = PendingStoreMessage.parse_obj(storage_metadata.message)


This call to parse_obj is not required anymore as storage_metadata must contain a valid PendingStoreMessage object already.

odesenfans · 2023-09-04T09:45:49Z

src/aleph/web/controllers/storage.py

+        item_content = json.loads(message.item_content)
+    actual_item_hash = sha256(content).hexdigest()
+    client_item_hash = item_content["item_hash"]
+    if len(content) > (1000 * MiB):


You read the file before checking the size. Doing the opposite would make a lot more sense, ex if size > (1000 * MiB) at the start of the function. You can also perform this check earlier to avoid checking the signature + user balance if the file is too large anyway.

odesenfans · 2023-09-04T10:21:17Z

src/aleph/web/controllers/storage.py

+            storage_metadata = StorageMetadata.parse_raw(metadata_content)
+        else:
+            storage_metadata = StorageMetadata.parse_raw(metadata)
+    except Exception as e:


Avoid wide except Exception clauses. What are you trying to intercept?

odesenfans · 2023-09-04T10:23:30Z

src/aleph/web/controllers/storage.py

+        if isinstance(metadata, FileField):
+            metadata_content = metadata.file.read()
+            storage_metadata = StorageMetadata.parse_raw(metadata_content)
+        else:


I'm pretty sure this else breaks the endpoint when the metadata is not specified. You default to b"" so you will get a Pydantic exception in this branch.

odesenfans · 2023-09-04T10:25:58Z

src/aleph/web/controllers/storage.py

+        if len(file_io.read()) > (25 * MiB):
+            raise web.HTTPUnauthorized()
+    file_io.seek(0)
+
    with session_factory() as session:
        file_hash = await storage_service.add_file(
            session=session, fileobject=file_io, engine=ItemType.storage
        )


Keeping this block for all cases means that you add the file directly in all cases before checking the signature, balance etc.

odesenfans · 2023-09-04T10:26:26Z

src/aleph/web/controllers/utils.py

+    try:
+        return parse_message(message_dict)
+    except InvalidMessageException as e:
+        raise web.HTTPUnprocessableEntity(body=str(e))


Nitpick: configure your IDE to add an empty line on save.

Co-authored-by: Olivier Desenfans <[email protected]>

This reverts commit f5c8e60.

1yam marked this pull request as ready for review August 22, 2023 14:32

odesenfans changed the title ~~1yam secure upload~~ Feature: authenticated file upload Aug 23, 2023

odesenfans requested changes Aug 23, 2023

View reviewed changes

1yam requested a review from odesenfans August 25, 2023 12:13

odesenfans requested changes Aug 28, 2023

View reviewed changes

1yam mentioned this pull request Aug 28, 2023

Feature: authenticated file upload SDK aleph-im/aleph-sdk-python#57

Closed

odesenfans requested changes Sep 4, 2023

View reviewed changes

BjrInt mentioned this pull request Sep 6, 2023

Support authenticated file upload aleph-im/aleph-sdk-ts#150

Closed

1yam mentioned this pull request Sep 28, 2023

Feature: Secure upload mechanism aleph-im/aleph-sdk-ts#153

Merged

1yam and others added 21 commits October 5, 2023 16:27

Features: Secure Endpoint

e0d201a

Features: Secure upload endpoint

d96ffa4

Fix : Clear test

cccb53f

Internal: add test for /add_file (200, 422, 402 tested)

3873b96

Fix: Mypy error

7367509

Refactor: Use MQ queue to trace status of the message

35a2c16

Fix: Test

ce83b88

Fix: the message was not commit to the db

41e1a86

Fix : Remove unused function

23ecf40

Refactor: Move private functions to public

b45ecaa

Fix: use ChainService instead of re create verify_signature

fdb8af9

Fix: missed to push it

1d1efb4

Fix: Use MiB instead of MB

55e2283

Fix

45809a1

Refactor: storage_add

455a4b8

Fix: Miss one conditions

11451e1

Fix: mypy error

85745cc

some Refactor

fa37f20

Fix: balances contrôle check all cost for user

79bd1f8

Fix: mypy error

76256d0

Update src/aleph/web/controllers/storage.py

553ee31

Co-authored-by: Olivier Desenfans <[email protected]>

1yam and others added 15 commits October 5, 2023 16:27

Fix: add_file functions who did not seek file object

68e4dba

Fix: conditions

ab95c92

Fix: Last bug & black

adc24d4

Fix: Contrôle balance only if file > 25 MiB

fa2c19e

Fix: conditions for balance control

ae73641

Fix: File < 25 MB so return code will be 200

fd3e64a

It's only a test

d54959b

Revert "It's only a test"

b8d9b17

This reverts commit f5c8e60.

Refactor: more opti

a1dc0ff

pair programming review

c7ea999

mypy fix 1

3d8a782

fix mypy

a6335b4

upload works again

127c4f0

nearly done, need mypy fixes + decide what to do with size test

fd27890

fix mypy and remove size test

ad2283f

odesenfans approved these changes Oct 5, 2023

View reviewed changes

odesenfans merged commit 3b5a30e into aleph-im:master Oct 6, 2023
2 checks passed

odesenfans deleted the 1yam_secure_upload branch October 6, 2023 08:57

1yam mentioned this pull request Oct 24, 2023

Feature: authenticated file upload SDK aleph-im/aleph-sdk-python#69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: authenticated file upload #463

Feature: authenticated file upload #463

1yam commented Aug 21, 2023 •

edited by odesenfans

Loading

odesenfans left a comment

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

1yam Aug 24, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

1yam Aug 24, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

odesenfans Aug 23, 2023

odesenfans Aug 25, 2023

odesenfans Aug 28, 2023

odesenfans Aug 28, 2023

1yam Aug 28, 2023

odesenfans Sep 4, 2023

odesenfans Aug 28, 2023

odesenfans Aug 28, 2023

odesenfans Aug 28, 2023

odesenfans left a comment

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

1yam Sep 5, 2023

odesenfans Sep 4, 2023

1yam Sep 5, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023

odesenfans Sep 4, 2023



		async def _verify_user_file(message: PendingStoreMessage, size: int, file_io) -> None:
		file_io.seek(0)



		async def storage_add_file_with_message(
		request: web.Request, session: DbSession, chain_service, storage_metadata, file_io

Feature: authenticated file upload #463

Feature: authenticated file upload #463

Conversation

1yam commented Aug 21, 2023 • edited by odesenfans Loading

odesenfans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odesenfans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1yam commented Aug 21, 2023 •

edited by odesenfans

Loading