-
Notifications
You must be signed in to change notification settings - Fork 41
#23/sjcl to webcrypto api #667
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
base: master
Are you sure you want to change the base?
Conversation
|
🎉 Beta deployment successful!: view the changes in live preview environment: https://gridbeta-github.asterics-foundation.org/alexangelov14/-23-sjcl-to-webcrypto-api/pr |
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.
Great that you've started with this 👍
Looks good at first sight, but since it's really something at the core and very important that it works, we should test very much. Did you already test with online users? You can use let _serverUrl = constants.IS_ENVIRONMENT_PROD || true in loginService.js to test with online users locally. Important are two tests:
- register / create user with the new crypto enabled
- register / create user with old crypto and test if it still works with the new implementation (fallback)
What we also could think about if it could be solved with this update is the issue I've explained in this comment: #541 (comment)
It would be great if we would get rid of metadata.id as encryption salt and maybe use the current username instead (which is much more stable and shouldn't change at all). So: keep as-is for the sjcl part, but don't use metadata.id as salt anymore for the new web crypto implementation.
| } | ||
|
|
||
| // Use SJCL for hash to maintain compatibility with existing hashes | ||
| // WebCrypto SHA-256 produces same result but we keep SJCL for consistency |
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.
Yeah, as long we need sjcl for backwards compatibilty, probably it makes sense to always use sjcl for hashes, since they're not the performance issue.
| */ | ||
| function getCrypto() { | ||
| if (typeof window !== 'undefined' && window.crypto) { | ||
| return window.crypto; |
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.
didn't know that web crypto is available in the main thread in window. When I looked at it some years ago, it was only available for webworkers, and that was the reason I never looked at it, it seemed to much work to implement.
Migration to WebCrypto api as requested in #23