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

key: Do not require users for parsing #1146

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

wiktor-k
Copy link
Contributor

This is a draft PR to get the CI results until I figure out why my npm test complains about missing asmcrypto modules :)

@twiss
Copy link
Member

twiss commented Aug 20, 2020

(Sorry for the force push - didn't realize this would happen. Please git reset --hard ec650e6 && git cherry-pick e2afcf1 or whatever your latest local commit was - it should apply cleanly.)

The CI passed, but I guess this means there were no keys without User IDs in the test suite. Perhaps it would be good to add one, and test that we can parse it but not encrypt with it?

Re. npm, you might have to npm install --only=dev to install the devDependencies, or something? 🤷‍♂️

@wiktor-k wiktor-k force-pushed the do-not-require-users-for-parsing branch from e2afcf1 to 3602a1e Compare August 20, 2020 13:18
@wiktor-k
Copy link
Contributor Author

(Sorry for the force push - didn't realize this would happen. Please git reset --hard ec650e6 && git cherry-pick e2afcf1 or whatever your latest local commit was - it should apply cleanly.)

I was just looking at the commit list wondering what happened... thanks for the instructions! 👍

The CI passed, but I guess this means there were no keys without User IDs in the test suite. Perhaps it would be good to add one, and test that we can parse it but not encrypt with it?

Yep, definitely. I'll be back with some more comprehensive test. Still I was surprised that not one test out of over 700 failed... 😱

Re. npm, you might have to npm install --only=dev to install the devDependencies, or something?

Install still fails with:

❯ npm install --only=dev
> openpgp@4.10.7 prepare /home/wiktor/src/open-source/openpgpjs
> npm run build
> openpgp@4.10.7 build /home/wiktor/src/open-source/openpgpjs
> rollup --config
src/index.js → dist/openpgp.js, dist/openpgp.min.js, dist/openpgp.mjs, dist/openpgp.min.mjs...
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
asmcrypto.js/dist_es8/aes/ecb (guessing 'ecb')
asmcrypto.js/dist_es8/hash/sha1/sha1 (guessing 'sha1$1')
asmcrypto.js/dist_es8/hash/sha256/sha256 (guessing 'sha256$1')
asmcrypto.js/dist_es8/aes/cfb (guessing 'cfb$1')
asmcrypto.js/dist_es8/aes/gcm (guessing 'gcm')
asmcrypto.js/dist_es8/aes/ctr (guessing 'ctr')
asmcrypto.js/dist_es8/aes/cbc (guessing 'cbc')
asmcrypto.js/dist_es8/aes/ecb (guessing 'ecb')
asmcrypto.js/dist_es8/hash/sha1/sha1 (guessing 'sha1$1')
asmcrypto.js/dist_es8/hash/sha256/sha256 (guessing 'sha256$1')
asmcrypto.js/dist_es8/aes/cfb (guessing 'cfb$1')
asmcrypto.js/dist_es8/aes/gcm (guessing 'gcm')
asmcrypto.js/dist_es8/aes/ctr (guessing 'ctr')
asmcrypto.js/dist_es8/aes/cbc (guessing 'cbc')
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
asmcrypto.js/dist_es8/aes/cfb (imported by src/crypto/cfb.js)
asmcrypto.js/dist_es8/aes/gcm (imported by src/crypto/gcm.js)
asmcrypto.js/dist_es8/aes/ctr (imported by src/crypto/eax.js)
...

but don't worry. I can create a test even without that and push it here and see it fail+pass. (it's weird that I ran the instructions even in docker run --rm -it -v ${PWD}:/code --entrypoint /bin/bash node and it still had some issues... oh, well... software 🤷‍♂️ )

Have a nice day! 👋

@wiktor-k wiktor-k force-pushed the do-not-require-users-for-parsing branch 2 times, most recently from fb2f3c5 to 02ab76d Compare August 21, 2020 10:53
@wiktor-k wiktor-k marked this pull request as ready for review August 21, 2020 11:12
@wiktor-k
Copy link
Contributor Author

Okay, phew. For such a tiny change I think this is now ready for review. Take a look at it @twiss and see if you can spot some places that are in need of improvement :)

Copy link
Member

@twiss twiss left a comment

Choose a reason for hiding this comment

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

👍 Thanks! ^.^

test/general/armor.js Outdated Show resolved Hide resolved
test/general/key.js Outdated Show resolved Hide resolved
@wiktor-k wiktor-k force-pushed the do-not-require-users-for-parsing branch from 02ab76d to 8a91818 Compare August 21, 2020 12:12
@twiss twiss merged commit fbac3df into openpgpjs:v5 Aug 21, 2020
@wiktor-k wiktor-k deleted the do-not-require-users-for-parsing branch August 25, 2020 10:35
twiss pushed a commit that referenced this pull request Sep 14, 2020
twiss pushed a commit that referenced this pull request Dec 12, 2020
larabr pushed a commit that referenced this pull request Jan 13, 2021
larabr pushed a commit that referenced this pull request Jan 13, 2021
twiss pushed a commit that referenced this pull request Jan 20, 2021
twiss pushed a commit that referenced this pull request Jan 24, 2021
twiss pushed a commit that referenced this pull request Feb 9, 2021
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

2 participants