-
Notifications
You must be signed in to change notification settings - Fork 3
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
transcode, sign and encrypt files for transmission #50
Conversation
…ode new lines and tabs
…nscode-sign-encrypt-files # Conflicts: # tests/kostentraeger/index.spec.ts
…nscode-sign-encrypt-files # Conflicts: # tests/kostentraeger/index.spec.ts
…nscode-sign-encrypt-files
…for transliteration, configured both for testing it with jest; added requirement of node >= 16 for using WebCrypto API; installed xmldom because DOMParser was no longer available; updated typescript and other dependencies and fixed typescript warnings and errors
…g certification requests
…e PDF that are no Kostenträger files and which should not be handled and parsed
# Conflicts: # src/sgb-xi/index.ts # src/sgb-xi/segments.ts # src/sgb-xi/validation.ts
This PR includes the grouping (by recipient), signature, encryption, transliteration and transcoding of Nutzdaten for transmission. It's grown into a rather large PR. Are you interested in reviewing it? |
Yes, I will |
okay, it's a lot... do you prefer one complete review with many comments that will take some time or do you prefer several smaller incomplete reviews? |
Thank you! Good question. I think the simpler, less time-consuming approach is appropriate. So that would be several smaller incomplete reviews, I assume? |
Yes |
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.
Alright, here is the first batch of comments. All relatively minor things I think and no mistakes or anything. At most, the duplicate PKI related dependencies could constitute a theoretical vulnerability (because the more deps, the higher the risk).
I mostly looked at files where you made only small changes and the PKI stuff and avoided the really big things because I am a lazy ass and just wanted to get this counter up ;-P
Once you resolved (or tapped on "resolve") all the comments, just tap the button to re-request a review and I'll have the next look.
Btw. in the interest of time efficiency, I suggest that if you think for any one thing "nah, it's fine", just hit the "resolve conversation" button without further explanation. |
Co-authored-by: Tobias Zwick <newton@openclonk.org>
Co-authored-by: Tobias Zwick <newton@openclonk.org>
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 will need to set aside some more time to get to the core of this PR (SGBXI message creation). For now, some more comments on various smaller things
Co-authored-by: Tobias Zwick <newton@openclonk.org>
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.
Allright, review done. I noticed that much of the bigger-seeming stuff was just moving code around from index.ts to calculation.ts and message.ts etc.
I also didn't look at the tests but just the code.
Thank you so much for taking the time to review! |
Closes #23, #20, #21 and #12