-
Notifications
You must be signed in to change notification settings - Fork 5
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
Support comments #13
Merged
Merged
Support comments #13
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
bf8706c
Add comment type
nain-F49FF806 8f15dd9
crypto: make decrypt output generic
nain-F49FF806 a0b7add
crypto: make decrypt input generic (may be paste or comment)
nain-F49FF806 1459ed3
crypto: remove now redundant convert code
nain-F49FF806 cd9280d
rename utility trait EncryptedT to "Decryptable"
nain-F49FF806 43963d2
some trait streamlining, and adata serialization fixes
nain-F49FF806 9fe8cac
cargo fmt formatting
nain-F49FF806 f05da8f
Add some documentation comments
nain-F49FF806 af7e48c
fix: remove accidentally committed manual debug println
nain-F49FF806 8416672
rename pasteorcomment -> decryptable
nain-F49FF806 dae09ea
add comments regarding custom serialization purpose
nain-F49FF806 f72b557
remove redundant explicit lifetime annotation
nain-F49FF806 14c0dbd
normalize paste/comment adata serialization
nain-F49FF806 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
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
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.
Why did we remove Serialize here and implement it ourselves?
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.
Normal serde serialisation results in a JSON object
{ .. }
. But Privatebin expects data (adata) to be a JSON array[ .. ]
.PBCLI currently stores
adata_str
as a separate property in paste struct. This was required because direct serialisation ofdata
outputs a JSON object string, which is not the original adata string (which is array).By using this custom serialisation, we get a json array string identical to the original adata string.
With this, we can do away with adata_str field too, and the custom
TryFrom<serde_json::Value> for Paste
which sets it.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.
As a demonstration, you can see we haven't added a
TryFrom
for comment. So adata_str value remains""
. Line84 is sufficient to reconstruct the correct adata string because of this custom array serializer.I was going to propose this way of handling adata, and removal of
adata_str
field in another PR. But now maybe we can discuss this here itself.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.
Suggested patch, using custom serialize.
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.
Hey,
yea - with the removal of adata_str that makes a lot more sense now :), and I like the idea.
Do you want to add that change as a commit in this PR?
In addition, did you see the lifetime comment I made above?
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 saw it only now, sorry for missing it earlier. Didn't realise the compiler would figure it out, so I started explicit, and then just carried on. I'll remove them :)
Sure, I can it here, unless you want it as separate PR. If so, let me know.
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.
Cool!
Yeah, lets add it here, so its coupled together with the added custom serialization. Apart from that it LGTM. When the Lifetime fix is made and the removal of adata_str commit is in, I would proceed to merge it.
Thanks for the contribution!
Best Regards
Mydayyy
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 have added commits for both removal of lifetimes and removal of adata_str.
Thanks for the review!
Best regards.