Skip to content
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

Tn/add export functionality #831

Merged
merged 16 commits into from May 10, 2023
Merged

Tn/add export functionality #831

merged 16 commits into from May 10, 2023

Conversation

TylerNoblett
Copy link
Collaborator

@TylerNoblett TylerNoblett commented May 8, 2023

Adds an export feature per the following features

A few things to note:

  • a user can export the evidence if the global flag for exporting is enabled AND if they are an admin (system, operation, or part of a group with admin privileges).
  • Re: denormlization, I removed the tag ID and the evidence UUID, but let me know if you'd like me to include the evidence UUID and I'll add that back.
  • I am saving the images as JPEG and all the other media as txt, but I'm not sure if the terminal recordings are supposed to be another file type; let me know what they should be saved as!
Screen.Recording.2023-05-08.at.2.00.41.PM.mov

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@@ -60,6 +61,11 @@ module.exports = (env, argv) => ({
filename: 'main-[contenthash].css',
chunkFilename: '[chunkhash].css',
}),
new webpack.DefinePlugin({
'process.env': {
'ENABLE_EVIDENCE_EXPORT': process.env.ENABLE_EVIDENCE_EXPORT == 'true',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thought that I just had is that I could include this global flag on the backend and incorporate it into UserCanExportData, since most of the global flags are on the backend and that way all of the logic for allowing a user to export is on the backend. I'll do that tomorrow morning

@jrozner
Copy link
Member

jrozner commented May 8, 2023

Overall looks good and works. A few changes:

  • We should probably move away from using the description as the filename of the evidence. They can be long with markdown which can lead to undesired behavior. Currently if you have a / in a description it will turn the file that is stored in the zip into a directory and not actually store the data. This maybe possible abuse create malicious zip files. Maybe we just use the UUID based filename or some other normalized one.
  • We should include the filename in the JSON that is exported
  • Code snippets are currently exported as a JSON blob. It would probably be nicer if we just output the content of it into the file instead. The extension of that actual file should probably match the language selected. It would be nice if we could retain the original filename in the the evidence JSON that's exported.

@TylerNoblett
Copy link
Collaborator Author

Overall looks good and works. A few changes:

  • We should probably move away from using the description as the filename of the evidence. They can be long with markdown which can lead to undesired behavior. Currently if you have a / in a description it will turn the file that is stored in the zip into a directory and not actually store the data. This maybe possible abuse create malicious zip files. Maybe we just use the UUID based filename or some other normalized one.
  • We should include the filename in the JSON that is exported
  • Code snippets are currently exported as a JSON blob. It would probably be nicer if we just output the content of it into the file instead. The extension of that actual file should probably match the language selected. It would be nice if we could retain the original filename in the the evidence JSON that's exported.

For code snippets, I added:

  • a field that shows the name of the source file (if it exists)
  • the appropriate file extension*

Other:

  • The filename of the piece of evidence is included in the JSON in evidence.json
  • I'm now using the UUID of the filename for each piece of evidence
  • In frontend/src/components/code_block/supported_languages.ts, C and C++ are combined into the same grouping ({ name: "C / C++", value: "c_cpp" }), so I can't accurately save the file type for both (since the label doesn't delineate), so both of those are saved a {filename}.c, but let me know if you have any other thoughts!

@jrozner
Copy link
Member

jrozner commented May 9, 2023

Those changes look good. If C/C++ are the only case where the extension isn't clear that seems fine to just use .c. If there are many cases where we don't know the exact extension, let's just not add an extension and simplify it.

One other change, can we remove the extra tag information (eg. color) and just make it an array of the tags themselves (eg. ["tag1", "tag2", "tag3"])?

@jrozner
Copy link
Member

jrozner commented May 9, 2023

Also, can we change the key from uuid to filename?

@TylerNoblett
Copy link
Collaborator Author

Got it! I've

  • removed the tag color
  • changed uuid to filename
  • I also added some extra logic to change the file extension to .cpp if it's a c/c++ file and the source file is listed with an extension of c++

@jrozner
Copy link
Member

jrozner commented May 10, 2023

I think there was a misunderstanding with the tags. Can you just extract all the names from the tags and make it an array of strings (of the names) rather than an array of objects.

@TylerNoblett
Copy link
Collaborator Author

I think there was a misunderstanding with the tags. Can you just extract all the names from the tags and make it an array of strings (of the names) rather than an array of objects.

oh gotcha - I just reread your original message

@jrozner jrozner merged commit 78b933c into main May 10, 2023
9 checks passed
@jrozner jrozner deleted the tn/add-export-functionality branch May 10, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants