-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature - Manage payloads #2989
Merged
elegantmoose
merged 6 commits into
mitre:master
from
jbaptperez:feature/manage-payloads
Jun 17, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
39784af
Updates API v2 GET payloads
jbaptperez 037a180
Updates API v2 GET payloads
jbaptperez e4fe33b
Adds API v2 POST payloads
jbaptperez d1eeba7
Adds API v2 DELETE payloads
jbaptperez e7ac139
Moves payload API schemas into a dedicated module
jbaptperez d8b03fc
Moves a payload file upload function at class level
jbaptperez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from marshmallow import fields, schema | ||
|
||
|
||
class PayloadQuerySchema(schema.Schema): | ||
sort = fields.Boolean(required=False, default=False) | ||
exclude_plugins = fields.Boolean(required=False, default=False) | ||
add_path = fields.Boolean(required=False, default=False) | ||
|
||
|
||
class PayloadSchema(schema.Schema): | ||
payloads = fields.List(fields.String()) | ||
|
||
|
||
class PayloadCreateRequestSchema(schema.Schema): | ||
file = fields.Raw(type="file", required=True) | ||
|
||
|
||
class PayloadDeleteRequestSchema(schema.Schema): | ||
name = fields.String(required=True) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, didnt notice this before but is this file saving method necessary over just using the file saving method from the File service (https://github.com/mitre/caldera/blob/master/app/service/file_svc.py#L65)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elegantmoose, relevant remark.
Actually, it would be good to use it but I cannot directly.
When defining the
post_payloads()
method (which indirectly calls__save_file()
), I used the@aiohttp_apispec.form_schema(PayloadCreateRequestSchema)
annotation in order to have a proper swagger form and documentation.However, this annotation (hidden function) calls
request.multipart()
before I even enter thepost_payloads()
method, to check the form according to the given schema.Then the file can be read from another special
request
dictionary object (see aiohttp_apispec documentation).I cannot read twice the multipart form using standard aiohttp methods because the second time, I get no data.
Therefore, I cannot the
file_svc.save_multipart_file_upload()
orfile_svc.save_file()
directly because they assume nothing has called theaiohttp
multipart methods yet.However, I can try to do one of the following changes:
file_svc.save_multipart_file_upload()
so that according to the given parameters, the file will be read the aiohttp standard way or using a special aiohttp_apispec object produced by the@aiohttp_apispec.form_schema
annotation,In both cases, I would try to use as maximum common code as possible.
Note that the
file_svc.save_multipart_file_upload()
method reads entirely the uploaded file and gives it as apayload
parameter tofile_svc.save_file()
, which can lead to performance issues in the case of big files, but that's another story.So tell me what you prefer (including another proposal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like 2.
But only if not a big lift. Can you take a quick look but cap your effort, dont want to hold this PR up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elegantmoose, I had a look to the
file_svc.py
code.It handles general file upload for other services or the (old v1?) API (I'm new to Cladera and I don't have the whole history).
Beside, this payload upload code looks the first API v2 file upload implementation (what is more, using aiohttp_apispec for swagger integration), and there are some global differences:
file_svc.py
(v1) handles file encoding (HTTP header) and encryption for storage as my v2 code doesn't,file_svc.py
(v1) looks to handle some particular cases (file storage path, file extension),file_svc.py
(however, the implementation I made should supports big file).Finally, I'm running out of time for this work.
Therefore, I propose the following:
file_svc.py
, with all features (file encoding, file encryption, aiohttp_apispec constraints for swagger).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elegantmoose, I pushed the field name change (see point 1. above).
I saw you merged from master in the feature/manage-payloads branch: I can rebase it (+
git push --force
) to avoid a complex history if you want before validating this PR.I'll create another PR for the GUI part (magma repository).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay yea, too much of additional lift for this PR. Thanks for looking into though.
Yea, feel free to rebase it.
Im happy with this PR, I would just like @clenk to do quick look over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elegantmoose, branch rebased and force pushed.